You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
approx equal can be more restricted than == #13218
This is similar to #13047 but is broader than that. In fact, the current PR that fixes that issue will only cause similar issues between other types. I'm very surprised that such major behavioral change may be introduced as a bug fix in a patch release.....
While I think it's wrong to make such change in a patch release, I do agree with using more exact comparison for boolean types. However, it seems over restricted when comparison using normal == produces True.
This makes the behavior of approx a bit difficult to reason about. I doubt if anyone would be asking for a more restricted comparison by adding approx...
That changed recently - I actually think that the change is good, because I think it's much worse when tests pass-when-the-shouldn't than fail-when-they-shouldn't: in the latter case you find out and can do something about it.
This should definitely be documented though, perhaps by adding a sentence to the top of the pytest.approx() docstring: "Note that unlike builtin equality, this function considers booleans unequal to numeric zero or one."
I know that is is changed recently and as I said, I agree using exact comparisons for Boolean is reasonable (I think its borderline unacceptable to do so in a patch release or even a minor release but that’s a different issue).
However, type exact comparisons is way more strict than exact equality which I believe was what the original issue was asking about. This introduce a significant inconsistency between Boolean type and other types that’s way more significant their corresponding value range would suggest. It’s unclear where this should end. Should integer get the same treatment? After all, they also take discrete values and not continuous ones.
The argument that raising more error is good also doesn’t make any sense. If raising more error / returning false is seen as a benefit, then why not make everything that’s not floating point fail when using approx. After all, someone calling approx on integer or boolean may be doing so unintentionally so the safe thing to do here should be simply erroring out for all cases there any non floating point values and just require the user to do something about it instead. Also note that the “do something” could involve replicating approx’s logic with nested data structures e.g. for comparing [True] and [1].
And fwiw, the fix #13132 for the linked issue shows how wrong the exact type comparison is. Unless there’s a standard way to query if a type should be treated as bool, adding special cases for third party types simply doesn’t make sense. I’m sure there are other libraries that implement such types (numba seems to have it previously). Are those types going to be added as special cases as well? If not, I don’t see why numpy should be singled out if it was not a direct dependency.
which does catch both builtins.bool and numpy.bool_ in a kinda logically consistent and dependency-agnostic way. (Of course it falls apart if someone implemented their booleans like def __str__(self) -> str: return 'T' if self else 'F', but still.)
While I do agree on principle that it doesn't make sense for numpy booleans to receive special treatment as a non-direct dep, FWIW the ship has sailed and much of pytest.approx is already engineered specifically with numpy compatibility in mind (e.g. _pytest.python_api.ApproxNumpy). So maybe special-casing numpy.bool_ isn't as bad (nor as much of) a precedent as it sounds, but a more general implementation (that is also more robust than what I have above) would be much welcome.
I was looking at this for a first issue and went to just change the docs but after reading them it seems already evident, although somewhat tucked away, that the approach that has been taken here --as also expressed in #9354-- is to fall back on strict equality in the case of "nonnumeric types". With that being said, I really don't see how that can be reconciled with @TTsangSC's edge case of
>>> True == pytest.approx(1)
True
or likewise
>>> False == pytest.approx(0)
True
It seems to me that this is actually a bug and warrants new tests. Would you agree?
Would you like to work on this issue, or I can try to fix it? I have already opened a PR for this issue and corrected the docstring for approx. However, I now see that there is more to address than just clarifying approx behavior.
So I've found out that tests have in fact already been added already in the previously mentioned pull request under the test_expecting_bool function, except that they've all been # noqa : E712 tagged. I think raising a separate issue for that matter is probably a good idea, which can improve on the changes in that aforementioned pr by just fixing the __eq__ method for ApproxScalar to account for its asymmetry.
As for the issue initially raised here, @natmokval I think it's still just a matter of changing the docs. I saw that the reviewer in your pr said they thought it should be mentioned somewhere lower down and I wanted to change some of the spelling too while I was looking so if it's alright I've made a pr myself as well.
As for the issue initially raised here, @natmokval I think it's still just a matter of changing the docs. I saw that the reviewer in your pr said they thought it should be mentioned somewhere lower down and I wanted to change some of the spelling too while I was looking so if it's alright I've made a pr myself as well.
Thanks @callummscott. I updated my PR and moved the mention that approx considers booleans unequal to numeric zero or one further down in the docstrings.
Activity
Workaround pytest breaking change
Zac-HD commentedon Mar 9, 2025
That changed recently - I actually think that the change is good, because I think it's much worse when tests pass-when-the-shouldn't than fail-when-they-shouldn't: in the latter case you find out and can do something about it.
This should definitely be documented though, perhaps by adding a sentence to the top of the
pytest.approx()docstring: "Note that unlike builtin equality, this function considers booleans unequal to numeric zero or one."yuyichao commentedon Mar 9, 2025
I know that is is changed recently and as I said, I agree using exact comparisons for Boolean is reasonable (I think its borderline unacceptable to do so in a patch release or even a minor release but that’s a different issue).
However, type exact comparisons is way more strict than exact equality which I believe was what the original issue was asking about. This introduce a significant inconsistency between Boolean type and other types that’s way more significant their corresponding value range would suggest. It’s unclear where this should end. Should integer get the same treatment? After all, they also take discrete values and not continuous ones.
The argument that raising more error is good also doesn’t make any sense. If raising more error / returning false is seen as a benefit, then why not make everything that’s not floating point fail when using approx. After all, someone calling approx on integer or boolean may be doing so unintentionally so the safe thing to do here should be simply erroring out for all cases there any non floating point values and just require the user to do something about it instead. Also note that the “do something” could involve replicating approx’s logic with nested data structures e.g. for comparing [True] and [1].
And fwiw, the fix #13132 for the linked issue shows how wrong the exact type comparison is. Unless there’s a standard way to query if a type should be treated as bool, adding special cases for third party types simply doesn’t make sense. I’m sure there are other libraries that implement such types (numba seems to have it previously). Are those types going to be added as special cases as well? If not, I don’t see why numpy should be singled out if it was not a direct dependency.
TTsangSC commentedon Mar 17, 2025
Hi, sorry for barging in, but here's my two cents as a fellow user.
builtins.boolandnumpy.bool_in a kinda logically consistent and dependency-agnostic way. (Of course it falls apart if someone implemented their booleans likedef __str__(self) -> str: return 'T' if self else 'F', but still.)numpybooleans to receive special treatment as a non-direct dep, FWIW the ship has sailed and much ofpytest.approxis already engineered specifically withnumpycompatibility in mind (e.g._pytest.python_api.ApproxNumpy). So maybe special-casingnumpy.bool_isn't as bad (nor as much of) a precedent as it sounds, but a more general implementation (that is also more robust than what I have above) would be much welcome.callummscott commentedon Apr 2, 2025
I was looking at this for a first issue and went to just change the docs but after reading them it seems already evident, although somewhat tucked away, that the approach that has been taken here --as also expressed in #9354-- is to fall back on strict equality in the case of "nonnumeric types". With that being said, I really don't see how that can be reconciled with @TTsangSC's edge case of
or likewise
It seems to me that this is actually a bug and warrants new tests. Would you agree?
Zac-HD commentedon Apr 2, 2025
I agree that both of your examples should return
False(by symmetry), and would love new tests to that effect.natmokval commentedon Apr 2, 2025
Hi @callummscott,
Would you like to work on this issue, or I can try to fix it? I have already opened a PR for this issue and corrected the docstring for
approx. However, I now see that there is more to address than just clarifyingapproxbehavior.callummscott commentedon Apr 2, 2025
So I've found out that tests have in fact already been added already in the previously mentioned pull request under the
test_expecting_boolfunction, except that they've all been# noqa : E712tagged. I think raising a separate issue for that matter is probably a good idea, which can improve on the changes in that aforementioned pr by just fixing the__eq__method forApproxScalarto account for its asymmetry.As for the issue initially raised here, @natmokval I think it's still just a matter of changing the docs. I saw that the reviewer in your pr said they thought it should be mentioned somewhere lower down and I wanted to change some of the spelling too while I was looking so if it's alright I've made a pr myself as well.
natmokval commentedon Apr 2, 2025
Thanks @callummscott. I updated my PR and moved the mention that
approxconsiders booleans unequal to numeric zero or one further down in the docstrings.Add title and change spelling for use of approx with non-numerics (#1…
Add title and change spelling for use of approx with non-numerics (#1…
Add title and change spelling for use of approx with non-numerics (#1…
Doc: clarify `approx` behavior regarding int and bools (#13341)
Doc: clarify `approx` behavior regarding int and bools (#13341) (#13352)
Doc: clarify `approx` behavior regarding int and bools (pytest-dev#13341
chore(deps): update dependency pytest to v9 (#305)