BUG: Fixed maximum relative error reporting in assert_allclose

[CakeWithSteak]

Fixed maximum relative error reporting in assert_allclose.

In cases where the two arrays have zeros at the same positions it will
no longer report nan as the max relative error.

Fixes #13801.

[charris]

Note to self: needs backport to 1.16.

[seberg]

Dang, I just went back to the original issue. The issue is really only for exact zeros? (everything else would not map to NaN. In that case, we could also use nonzero = y != 0 and print (error[nonzero] / abs(y[nonzero])).max(). The function uses advanced indexing anyway already to check the NaN positions.

[CakeWithSteak]

Yeah oops, you're right, I thought there would be more possibilities for the result to be nan.
Actually you can get nan one other way, by calling assert_allclose with equal_nan=False on two arrays that have nan at the same positions. In that case the absolute difference is also nan though, so I guess it's not a bug.
Example:

x = np.array([np.nan,1])
y = np.array([np.nan,2])
np.testing.assert_allclose(x,y, equal_nan=False)

Output:

AssertionError: 
Not equal to tolerance rtol=1e-07, atol=0

Mismatch: 100%
Max absolute difference: nan
Max relative difference: nan
 x: array([nan,  1.])
 y: array([nan,  2.])

Should I make a revert commit to revert the previous modifications, then add your changes in a separate one, or should I do it in just one commit?

[seberg]

don't worry about the history. If you want you can make a local backup branch just in case I guess.

[seberg]

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.

[seberg]

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

[CakeWithSteak]

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

[charris]

Close/reopen to rerun tests.

[charris]

There is a problem with the test.

[charris]

Pushing off to 1.17.2.

[CakeWithSteak]

#14203 changed the mismatch message in assert_array_compare, and that broke my test. I'll update it tomorrow when I get home.

[mattip]

LGTM. @seberg care to re-review?

[seberg]

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.

[CakeWithSteak]

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?

[seberg]

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

[CakeWithSteak]

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

[seberg]

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

[CakeWithSteak]

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

[charris]

@seberg Look good to you?

[seberg]

Yes, looks good. Thanks @CakeWithSteak.