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.

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

Jump to bottom

BUG: Fixed maximum relative error reporting in assert_allclose #13802

Merged
merged 5 commits into from
Sep 5, 2019
Merged

BUG: Fixed maximum relative error reporting in assert_allclose #13802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
741e6b6
Fixed maximum relative error reporting in assert_allclose:
CakeWithSteak Jun 18, 2019
26ed51a
Changed assert_array_compare to avoid usage of a warnings context.
CakeWithSteak Aug 6, 2019
5d2aee7
Simplified the previous fix
CakeWithSteak Aug 6, 2019
08bca18
MAINT: Changed test_report_max_relative_error to use a more specific …
CakeWithSteak Aug 23, 2019
d5cad8c
MAINT: Ensure that np.all is called instead of builtins all in assert…
CakeWithSteak Aug 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions numpy/testing/_private/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,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, array2string, isnan, inf, bool_, errstate
from numpy.core import array, array2string, isnan, inf, bool_, errstate, all

x = array(x, copy=False, subok=True)
y = array(y, copy=False, subok=True)
Expand Down Expand Up @@ -821,7 +821,12 @@ def func_assert_same_pos(x, y, func=isnan, hasval='nan'):

# note: this definition of relative error matches that one
# used by assert_allclose (found in np.isclose)
max_rel_error = (error / abs(y)).max()
# Filter values where the divisor would be zero
nonzero = bool_(y != 0)
if all(~nonzero):
Copy link
Member

Choose a reason for hiding this comment

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

nonzero is guaranteed to be an array here I think, so it would be better to use nonzero.all() (the other one may be pythons all). Other than that, seems all fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nonzero isn't necessarily an array, for example in test TestArrayEqual.test_subclass_that_overrides_eq the not equal operator returns a single bool - calling .all() on it would raise an AttributeError. You're right about all being python's all - I'll import the function from numpy to fix that.

Also, applying operator ~ to a bool wouldn't work as intended, so I think it would be better to use any:

nonzero = y != 0
if not any(nonzero):
	max_rel_error = array(inf)
else:
	max_rel_error = (error[nonzero] / abs(y[nonzero])).max()

That would work even if the not equal operator returns a single bool, but it's kind of hard to read, so we could also invert the if statement.
Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

A numpy bool woul dbe good enough and considering that indexing works below it should do. But np.all is fine as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, simply this?

nonzero = bool_(y != 0)
if all(~nonzero):
	max_rel_error = array(inf)
else:
	max_rel_error = (error[nonzero] / abs(y[nonzero])).max()

Copy link
Member

Choose a reason for hiding this comment

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

Just make it np.any and I will be happy (or tell me that it already uses np.any).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean np.all here? Yes, all is imported at the top of the function.

max_rel_error = array(inf)
Copy link
Member

Choose a reason for hiding this comment

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

OK, I guess this seems right if y happens to be all zero. I believe the result here should be just inf and not an array with inf inside, max also returns a scalar. (Although this is probably just printed, so it may not even make a difference.

Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test for this branch (assuming it does not exist)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

max_rel_error is passed to array2string and then printed, that's why I made it an array (although I guess float_(inf) would also work, but it makes no difference)
For the inf branch there is already a test: testing.tests.test_utils.TestAlmostEqual.test_error_message which tests this exact scenario of y being all zeros.
I've also written a test for the original issue TestAssertAllclose.test_report_max_relative_error

else:
max_rel_error = (error[nonzero] / abs(y[nonzero])).max()
if error.dtype == 'object':
remarks.append('Max relative difference: '
+ str(max_rel_error))
Expand Down
9 changes: 9 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 @@ -880,6 +880,15 @@ def test_equal_nan_default(self):
assert_array_less(a, b)
assert_allclose(a, b)

def test_report_max_relative_error(self):
a = np.array([0, 1])
b = np.array([0, 2])

with pytest.raises(AssertionError) as exc_info:
assert_allclose(a, b)
msg = str(exc_info.value)
assert_('Max relative difference: 0.5' in msg)


class TestArrayAlmostEqualNulp(object):

Expand Down