Implemented Quantity-aware wrapper for assert_allclose

[astrofrog]

This is a replacement for #2402. It seems there is general resistance against having quantity-aware wrappers for many Numpy functions, but the one here is one that is needed in various packages so it makes sense to make it available as a helper.

Not a full feature, just part of the infrastructure, so I figured this would be ok for 1.0?

[Cadair]

👍

[mhvk]

My resistance is due mostly to the fact that I feel this is a bug in numpy, and it would be good to try to improve numpy rather than work around it. Specifically, assert_allclose would become fine if one had at least the option to allow subclasses in numpy.core.numeric._allclose_points (https://github.com/numpy/numpy/blob/master/numpy/core/numeric.py#L2205 and two further instances down from there).

Could you perhaps raise a wishlist issue with numpy about that?

But having written many tests with allclose and being annoyed, I do see the point of having the wrapper now. Comments on the implementation will be inline.

[mhvk]

I feel this is excessive testing of inputs! Elsewhere, anything not a Quantity is treated as dimensionless, and I would just do that here too, raising UnitsError if necessary. So, I would do:

actual = Quantity(actual, subok=True, copy=False)
desired = Quantity(desired, subok=True, copy=False).to(actual.unit)
atol = Quantity(atol, subok=True, copy=False).to(actual.unit)
rtol=  Quantity(rtol, subok=True, copy=False).to(u.dimensionless_unscaled)
np.testing.assert_allclose(actual.value, desired.value,
                           rtol=rtol.value, atol=atol.value, err_msg=err_msg, verbose=verbose)

[astrofrog]

I don't mind, but if we do this, the error messages will be less explicit than they currently are. What do others think?

[mhvk]

But very similar to the error messages you get generally from working with quantities...

[eteq]

I agree with @mhvk that this may be a call to try to get more explicit error messages in Quantity itself... But I think there's no harm in leaving this in to make reading of test results easier.

[astrofrog]

Actually I have implemented this, but I override the exceptions if any are raised, which I think is a good compromise

[embray]

Certainly for testing purposes I see no problem with having this.

[eteq]

You should probably note in the docstring here that the definition of atol is set by the unit of actual, not desired (because desired gets converted to actual.units). Or even go a bit further and change the name of atol to atol_actualunit or something like that.

[astrofrog]

Actually it doesn't matter: atol has to be specified as a quantity to work so the units have to be explicit anyway (and can be different from actual.unit. The only reason it doesn't have units by default is because for zero it's unambiguous and doesn't matter what the units are. But you can do:

assert_quantity_allclose([1,2] * u.m, [100,200] * u.cm, atol=2 * u.cm)

and that should work. In fact, if atol is non-zero, you have to give units.

[eteq]

Aside from the minor comments, I'm 👍 to this, @astrofrog

[mhvk]

Tiny really, but somehow this feels slightly confusing. How about writing

# we take 0 to be convertible to any unit
atol = u.Quantity(0, actual.unit)

You could also consider making the default atol=None and then test for that, i.e.,

if atol is None:
    # by default, we assume an absolute tolerance of 0
    atol = u.Quantity(0, actual.unit)
else:
    ...

[astrofrog]

Fixed

[mhvk]

@astrofrog - yes, I'm fine with catching the Quantity exceptions. As @embray noted, perhaps the fact that you feel it is helpful to do so is a signal that the exceptions raised by Quantity itself need clarification...

Note my tiny comment -- implement it if you wish -- happy either way.