BUG: Fixed behavior of assert_array_less for +/-inf #8410
Conversation
|
Is there anything for me to do first, or can I just wait now for someone to review my PR? Sorry if this is a silly question or the wrong place to ask, as it is my first time trying to contribute to numpy. |
|
No, you can only remind us once in a while or try to convince everyone that this is vital, unless you know someone who can already start reviewing a little ;). Unfortunately review time is rather short in general.... |
|
Thanks for the quick reply, good to know. I was just wondering if I might have forgotten something so I thought I'd ask. Vital might be a stretch it is more of an edge case, it will just have to wait then. |
There was a problem hiding this comment.
Looks good to me. If you want to be thorough, could add a higher dimensional test and maybe a test to point out that -inf < -inf is also an error (maybe its there already and I missed it).
Since it is a change in behaviour (even if bug fix) that other tests suits could run into, a short mention in the release notes might be good (we can always edit those later again before the release).
| class TestArrayAssertLess(unittest.TestCase): | ||
|
|
||
| def setUp(self): | ||
| self._assert_func = assert_array_less |
There was a problem hiding this comment.
Not much reason for this unless there is some form of inheritance somewhere, but if you like it...
There was a problem hiding this comment.
I don't have any strong opinion concerning this. I was just trying to follow the surrounding style as TestApproxEqual is also using it without inheriting from anywhere and not being inherited from. As it doesn't make the code less understandable I would tend to leave it in, but if you feel that it should be dropped, I can also do that.
There was a problem hiding this comment.
Have a slight preference only, so whichever you like. Makes me a bit wonder whether TestApproxEqual probably should inherit from _GenericTest, though, hehe.
There was a problem hiding this comment.
Really, release notes, and maybe double check if the tests cover all important and it should be good.
| __tracebackhide__ = True # Hide traceback for py.test | ||
| from numpy.core import array, isnan, isinf, any, all, inf | ||
| from numpy.core import array, isnan, isinf, any, all, inf, zeros_like | ||
| from numpy.core.numerictypes import bool_ |
There was a problem hiding this comment.
@charris do you know why we have the imports here always? Circular imports I guess? Could put it to the top of the file, but again, seems fine can stay for another time.
| @@ -930,7 +931,7 @@ def compare(x, y): | |||
| if npany(gisinf(x)) or npany( gisinf(y)): | |||
| xinfid = gisinf(x) | |||
| yinfid = gisinf(y) | |||
| if not xinfid == yinfid: | |||
| if not (xinfid == yinfid).all(): | |||
There was a problem hiding this comment.
Out of curiousity, why did this not create an error before? It seems obviously wrong?
There was a problem hiding this comment.
Actually, this code was never called for arrays containing either np.inf, -np.inf, or np.nan as these were filtered before by assert_array_compare. It is obviously wrong though and surfaces as soon as you allow the compare method to actually compare np.inf's by the new equal_inf=False option of assert_array_compare.
|
A few small comments, nothing much though, it was really a very good PR, good tests coverage, etc. |
jotasi
force-pushed
the
arrayAssertLessFix
branch
2 times, most recently
from
93c910c
to
6290d89
Compare
Added tests for simple arrays/elements, tests that ensure that nans are not compared when they are in the same position in both arrays but raise AssertionError when they are not, and tests that ensure inf logic (-np.inf < np.inf, 1.0 < np.inf, -np.inf < 1.0) and that all other cases fail.
This failed to raise AssertionError in an earlier rewrite of assert_array_compare and is thus tested explicitly now.
See issue numpy#2418 assert_array_less does now treat inf/nan correctly, namely: -inf < x < inf for any real number x nans still are not compared, when they occur at the same position To achieve this without breaking any other functions relying on assert_array_compare, an additional parameter (equal_inf) was introduced that defaults to the old behavior (analogous to equal_nan).
The bug that +/- inf where not treated correctly in np.testing.assert_array_less changed the behavior and is thus mentioned in the release notes.
jotasi
force-pushed
the
arrayAssertLessFix
branch
from
6290d89
to
6312db5
Compare
|
Thanks for the review and the nice comments! You were right, I forgot to check that |
|
Good enough for me. The release notes get a bit extra work anyway before the actual release. And the style stuff, can be done when the whole file gets some updates ;). Thanks again! |
|
Also this kind of change.... better to do it early in the release cycle, so that others notice if it screws with their tests.... |
|
Perfect. Thanks for your help @seberg! |
|
This PR is causing numerous test failures on astropy; see #8452 for a fix. |
numpygh-8410 breaks a large number of astropy tests, because it sets up a boolean array for values that should actually be compared (i.e., are not `nan` or `inf`) using `zeros_like`. The latter means that for subclasses, the boolean test array is not a plain `ndarray` but the subclass. But for astropy's `Quantity`, the `all` method is undefined. This commit ensures the test arrays from `isinf` and `isnan` are used directly.
At the moment
assert_array_lessinnp.testingdoes not correctly treat +/-inf (see #2418). This is fixed by changing the underlying call toassert_array_compare, which now has an parameterequal_infto compare infs rather than to ignore them analogous to the already present parameterequal_nan. The default is set to reproduce the old behavior and thus to not break existing code.