8000 BUG,MAINT: Ensure masked elements can be tested against nan and inf. by mhvk · Pull Request #11122 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 7, 2018
Merged

BUG,MAINT: Ensure masked elements can be tested against nan and inf. #11122

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5718b33
BUG,MAINT: Ensure masked elements can be tested against nan and inf.
mhvk May 19, 2018
3ad49aa
MAINT: clean up assert_array_compare a bit further.
mhvk May 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions numpy/lib/tests/test_ufunclike.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,10 @@ def __array_wrap__(self, obj, context=None):
obj.metadata = self.metadata
return obj

def __array_finalize__(self, obj):
self.metadata = getattr(obj, 'metadata', None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe in the commit message why these changes were needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. This was indeed a non-trivial one - I had to look again to remind me why it was needed. I wondered about the use of stating it in the commit message until I recalled that with git blame one can find why this was added.

return self

a = nx.array([1.1, -1.1])
m = MyArray(a, metadata='foo')
f = ufl.fix(m)
Expand Down
76 changes: 35 additions & 41 deletions numpy/testing/_private/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, inf, bool_
x = array(x, copy=False, subok=True)
y = array(y, copy=False, subok=True)

Expand All @@ -695,17 +695,28 @@ def isnumber(x):
def istime(x):
return x.dtype.char in "Mm"

def chk_same_position(x_id, y_id, hasval='nan'):
"""Handling nan/inf: check that x and y have the nan/inf at the same
locations."""
try:
assert_array_equal(x_id, y_id)
except AssertionError:
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."""
# Both the != True comparison here and the cast to bool_ at
# the end are done to deal with `masked`, which cannot be
# compared usefully, and for which .all() yields masked.
x_id = func(x)
y_id = func(y)
if (x_id == y_id).all() != True:
Copy link
Member
@charris charris Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm,

In [10]: ma.masked != True
Out[10]: masked

In [11]: ma.masked == True
Out[11]: masked

In [12]: bool(ma.masked)
Out[12]: False

@mhvk The camparison to True seems to achieve nothing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose is to ensure that the stanza is not executed if x_id or y_id is masked, yet is executed if they are different.

msg = build_err_msg([x, y],
err_msg + '\nx and y %s location mismatch:'
% (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.
if x_id.ndim == 0:
return bool_(x_id)
elif y_id.ndim == 0:
return bool_(y_id)
else:
return y_id

try:
cond = (x.shape == () or y.shape == ()) or x.shape == y.shape
Expand All @@ -718,49 +729,32 @@ def chk_same_position(x_id, y_id, hasval='nan'):
names=('x', 'y'), precision=precision)
raise AssertionError(msg)

flagged = bool_(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,
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)

if any(x_isnat) or any(y_isnat):
chk_same_position(x_isnat, y_isnat, hasval="NaT")
flagged = func_assert_same_pos(x, y, func=isnat, hasval="NaT")

if any(x_isnat) or any(y_isnat):
x = x[~x_isnat]
y = y[~y_isnat]
if flagged.ndim > 0:
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I agree with this - what if x and y are quantities of different dimensions? The test should still compare the empty arrays so that the correct error is raised, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the same case as the if x.size == 0 stanza above, but for the case where x and y are both scalar. In principle, I agree the comparison should work for empty elements, but in practice, this is not the case (e.g., matrix changing its shape being one problem...)

Since this was here before, I propose to leave it as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, i agree this is out of scope for the patch

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

Expand Down
18 changes: 18 additions & 0 deletions numpy/testing/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,17 @@ def test_recarrays(self):
self._test_not_equal(c, b)
assert_equal(len(l), 1)

def test_masked_nan_inf(self):
# Regression test for gh-11121
a = np.ma.MaskedArray([3., 4., 6.5], mask=[False, True, False])
b = np.array([3., np.nan, 6.5])
self._test_equal(a, b)
self._test_equal(b, a)
a = np.ma.MaskedArray([3., 4., 6.5], mask=[True, False, False])
b = np.array([np.inf, 4., 6.5])
self._test_equal(a, b)
self._test_equal(b, a)


class TestBuildErrorMessage(object):

Expand Down Expand Up @@ -390,6 +401,9 @@ def test_subclass_that_cannot_be_bool(self):
# comparison operators, not on them being able to store booleans
# (which, e.g., astropy Quantity cannot usefully do). See gh-8452.
class MyArray(np.ndarray):
def __eq__(self, other):
return super(MyArray, self).__eq__(other).view(np.ndarray)

def __lt__(self, other):
return super(MyArray, self).__lt__(other).view(np.ndarray)

Expand Down Expand Up @@ -489,6 +503,9 @@ def test_subclass_that_cannot_be_bool(self):
# comparison operators, not on them being able to store booleans
# (which, e.g., astropy Quantity cannot usefully do). See gh-8452.
class MyArray(np.ndarray):
def __eq__(self, other):
return super(MyArray, self).__eq__(other).view(np.ndarray)

def __lt__(self, other):
return super(MyArray, self).__lt__(other).view(np.ndarray)

Expand Down Expand Up @@ -650,6 +667,7 @@ def test_inf_compare_array(self):
assert_raises(AssertionError, lambda: self._assert_func(-ainf, -x))
self._assert_func(-ainf, x)


@pytest.mark.skip(reason="The raises decorator depends on Nose")
class TestRaises(object):

Expand Down
0