-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
BUG,MAINT: Ensure masked elements can be tested against nan and inf. #11122
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
The removal of nan and inf from arrays that are compared using test routines like assert_array_equal treated the two arrays separately, which for masked arrays meant that some elements would not be removed when they should have been. This PR corrects this.
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -685,7 +685,7 @@ def assert_array_compare(comparison, x, y, err_msg='', verbose=True, | |
header='', precision=6, equal_nan=True, | ||
equal_inf=True): | ||
__tracebackhide__ = True # Hide traceback for py.test | ||
from numpy.core import array, isnan, isinf, any, inf | ||
from numpy.core import array, isnan, any, inf, ndim | ||
x = array(x, copy=False, subok=True) | ||
y = array(y, copy=False, subok=True) | ||
|
||
|
@@ -695,9 +695,14 @@ def isnumber(x): | |
def istime(x): | ||
return x.dtype.char in "Mm" | ||
|
||
def chk_same_position(x_id, y_id, hasval='nan'): | ||
8000 """Handling nan/inf: check that x and y have the nan/inf at the same | ||
locations.""" | ||
def func_assert_same_pos(x, y, func=isnan, hasval='nan'): | ||
"""Handling nan/inf: combine results of running func on x and y, | ||
checking that they are True at the same locations.""" | ||
x_id = func(x) | ||
y_id = func(y) | ||
if not any(x_id) and not any(y_id): | ||
return False | ||
|
||
try: | ||
assert_array_equal(x_id, y_id) | ||
except AssertionError: | ||
|
@@ -706,6 +711,9 @@ def chk_same_position(x_id, y_id, hasval='nan'): | |
% (hasval), verbose=verbose, header=header, | ||
names=('x', 'y'), precision=precision) | ||
raise AssertionError(msg) | ||
# If there is a scalar, then here we know the array has the same | ||
# flag as it everywhere, so we should return the scalar flag. | ||
return x_id if x_id.ndim == 0 else y_id | ||
|
||
try: | ||
cond = (x.shape == () or y.shape == ()) or x.shape == y.shape | ||
|
@@ -718,49 +726,32 @@ def chk_same_position(x_id, y_id, hasval='nan'): | |
names=('x', 'y'), precision=precision) | ||
raise AssertionError(msg) | ||
|
||
flagged = False | ||
if isnumber(x) and isnumber(y): | ||
has_nan = has_inf = False | ||
if equal_nan: | ||
x_isnan, y_isnan = isnan(x), isnan(y) | ||
# Validate that NaNs are in the same place | ||
has_nan = any(x_isnan) or any(y_isnan) | ||
if has_nan: | ||
chk_same_position(x_isnan, y_isnan, hasval='nan') | ||
flagged = func_assert_same_pos(x, y, func=isnan, hasval='nan') | ||
|
||
if equal_inf: | ||
x_isinf, y_isinf = isinf(x), isinf(y) | ||
# Validate that infinite values are in the same place | ||
has_inf = any(x_isinf) or any(y_isinf) | ||
if has_inf: | ||
# Check +inf and -inf separately, since they are different | ||
chk_same_position(x == +inf, y == +inf, hasval='+inf') | ||
chk_same_position(x == -inf, y == -inf, hasval='-inf') | ||
|
||
if has_nan and has_inf: | ||
x = x[~(x_isnan | x_isinf)] | ||
y = y[~(y_isnan | y_isinf)] | ||
elif has_nan: | ||
x = x[~x_isnan] | ||
y = y[~y_isnan] | ||
elif has_inf: | ||
x = x[~x_isinf] | ||
y = y[~y_isinf] | ||
|
||
# Only do the comparison if actual values are left | ||
if x.size == 0: | ||
return | ||
flagged |= func_assert_same_pos(x, y, | ||
func=lambda xy: xy == +inf, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Learn something new every day... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But doesn't work for complex numbers... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does what you have here work for complex numbers? |
||
hasval='+inf') | ||
flagged |= func_assert_same_pos(x, y, | ||
func=lambda xy: xy == -inf, | ||
hasval='-inf') | ||
|
||
elif istime(x) and istime(y): | ||
# If one is datetime64 and the other timedelta64 there is no point | ||
if equal_nan and x.dtype.type == y.dtype.type: | ||
x_isnat, y_isnat = isnat(x), isnat(y) | ||
flagged = func_assert_same_pos(x, y, func=isnat, hasval="NaT") | ||
|
||
if any(x_isnat) or any(y_isnat): | ||
chk_same_position(x_isnat, y_isnat, hasval="NaT") | ||
|
||
if any(x_isnat) or any(y_isnat): | ||
x = x[~x_isnat] | ||
y = y[~y_isnat] | ||
if ndim(flagged): | ||
x, y = x[~flagged], y[~flagged] | ||
# Only do the comparison if actual values are left | ||
if x.size == 0: | ||
return | ||
elif flagged: | ||
# no sense doing comparison if everything is flagged. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure I agree with this - what if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is the same case as the Since this was here before, I propose to leave it as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, i agree this is out of scope for the patch There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Overall, it sadly remains a chunk of rather fragile code... |
||
return | ||
|
||
val = comparison(x, y) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these two lines needed? Aren't they covered by the below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd think not, but numerous recursion failures if I leave them out...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what happens is that it does recurs 8000 e, but the
if
statement prevents it from doing it more than once.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which is simple to solve - just use
any
- will push fix up shortly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't work either, but passing in
equal_[nan|inf]=False
does.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment saying that this is needed to prevent recursion or something?
Good to go either way