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

False positive unnecessary-negation for float comparison #9741

Open
NicoB77 opened this issue Jun 21, 2024 · 2 comments
Open

False positive unnecessary-negation for float comparison #9741

NicoB77 opened this issue Jun 21, 2024 · 2 comments
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection

Comments

@NicoB77
Copy link

NicoB77 commented Jun 21, 2024

Bug description

The code below causes unnecessary-negation, which is wrong. For float comparisons, "not a < b" and "b <= a" are not equivalent: if a or b is nan, the first condition is true while the second is false.

target = float(target)
if not 0.0 < target:
    raise ValueError('Target must be positive!')

Command used

pylint --disable=RP0001 --disable=RP0003 --disable=RP0101 --disable=RP0401 --disable=RP0701 --disable=RP0801

Pylint output

************* Module a
a.py(134): [C0117 A.B] Consider changing "not 0.0 < target" to "0.0 >= target" (unnecessary-negation)

Expected behavior

No warning

Pylint version

pylint 3.2.3
astroid 3.2.2
Python 3.10.6 (tags/v3.10.6:9c7b4bd, Aug  1 2022, 21:53:49) [MSC v.1932 64 bit (AMD64)]
@NicoB77 NicoB77 added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Jun 21, 2024
@Pierre-Sassoulas
Copy link
Member

Thank you for opening the issue.

I feel like the nan should be handled in another conditional because explicit is better than implicit:

target = float(target)
if math.isnan(target) or target <= 0.0:
    raise ValueError('Target must be positive!')

Not sure if we should upgrade the check's message to add math.isnan to all the suggestions though. Might be a little verbose and not relevant most of the time. Or it might be a nice "beware of nan" reminder that actually helps user ?

@Pierre-Sassoulas Pierre-Sassoulas added False Positive 🦟 A message is emitted but nothing is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Jun 21, 2024
@NicoB77
Copy link
Author

NicoB77 commented Jun 24, 2024

Thanks for your reply. The explicit call of isnan looks too verbose to me, I would rather not add this to the suggestions. Since the usual equivalences of comparisons do not hold for floats, I am not sure if there should be a warning in the first place. As a workaround I have disabled this warning for my project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Positive 🦟 A message is emitted but nothing is wrong with the code Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Development

No branches or pull requests

2 participants