Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confusion in test_label_eligible apply_reflexive_mapping #40

Open
TonyStew opened this issue Nov 7, 2024 · 1 comment
Open

Confusion in test_label_eligible apply_reflexive_mapping #40

TonyStew opened this issue Nov 7, 2024 · 1 comment

Comments

@TonyStew
Copy link

TonyStew commented Nov 7, 2024

In core.test_label_eligible we're calling _test_label_disposition like so:
(disposition, action_idx) = self._test_label_disposition(label, apply_reflexive_mapping=not is_variant)

But the apply_reflexive_mapping param of _test_label_disposition is described as:

apply_reflexive_mapping: Whether the reflexive mapping should be considereed for disposition
(This should be True when evaluating a variant)

Is it correct that we're inverting is_variant here? That seems contradictory with the comment.

@j-bernard
Copy link
Contributor

Thanks for reporting this.

IIRC, when doing the implementation, the logic was initially inverted. So it seems the comment is tied to the initial implementation.

The associated commit message matches with the logic that is implemented:

Stop considering reflexive mapping when evaluating a variant label

Will fix the comment for the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants