I am perfectly happy with the actual fix to the test functions here (indeed, the code is cleaner than before), but I think it is absolute insanity for numpy functions to support subclasses that explicitly disable built-in methods like all.

assert_almost_equal (and other numpy functions) should either coerce to base numpy arrays or work for well behaved subclasses, but it's not realistic for NumPy to cater to the quirks of every strange ndarray subclasses. AstroPy should use its own function for this.

My preference would be to make this change, but leave out the unit tests for the ndarray subclass without the all method. Yes, this means that this will likely break again at some point in the future. Such are the hazards of subclassing ndarray.

@shoyer - I agree that the test at some level is rather odd, even though it is quite obvious that there will be other subclasses for which a boolean dtype simply does not make sense but for which comparisons are well defined; in a sense, the problem here really was that zeros_like allows one to override the dtype, which to me makes very little sense.

But while I don't quite like my own test either, it does seem this is a regression one might as well ensure does not recur; it doesn't seem that unlikely to me there are others than astropy that rely on this working. Would the test be less odd to you if the sample subclass had an explicit __array_finalize__ that raised an error if the dtype was boolean?

p.s. We do have our own assert_quantity_allclose since the numpy version does not work for non-dimensionless quantities. The fact that nonetheless many tests failed just goes to show how wonderfully well (but sometimes frustratingly so) duck-typing often works.

Well, not putting a test because we don't want to necessary take care to not break it is not too useful IMHO. Might as well just say in a comment that the test documents a behaviour but is not considered guaranteed. I realize that some people may be discouraged by a failing test immidiately, but my guess is you should always look what kind of test you are breaking and then you see the comment.

@shoyer - would a comment in the test along the lines of what @seberg suggested be OK with you?

@mhvk

would a comment in the test along the lines of what @seberg suggested be OK with you?

Yes, that's fine.

To be clear my complaint here is really more theoretical than practical. As long as you are testing against numpy master and promptly supplying patches when things break, keeping numpy functions working for AstroPy's ndarray subclasses is totally reasonable (assuming no significant performance or code complexity costs).

I just don't want to hobble NumPy with a commitment to supporting this behavior in the long term. The nature of subclassing in Python is that without a clearly delimited public API for subclasses (which doesn't exist for NumPy) it is extremely difficult (in practice impossible) to avoid leaking implementation details in the API. So this sort of breakage happens almost inevitably.

OK, all makes sense. I pushed a version with a revised comment. With that, I think this is ready to go in.

Hehe, the comment could be more obvious about not being too afraid of making the test fail, but OK. Thanks!

Yeah, surprising numpy tests did not notice. Possibly the always 2D stuff breaks things, matrices don't quite support the boolean indexing after all. @mhvk can you check it out?

I guess this is the case. You can trigger the error easily by doing:

import numpy as np
np.assert_array_equal(np.matrix([[np.nan, np.inf]]), np.array([[np.nan, np.inf]]))

This will give the following error message:

---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-2-32896f34a9e8> in <module>()
----> 1 np.testing.assert_array_equal(np.matrix([[np.nan, np.inf]]), np.array([[np.nan, np.inf]]))

XXX/numpy/testing/utils.pyc in assert_array_equal(x, y, err_msg, verbose)
    841     """
    842     assert_array_compare(operator.__eq__, x, y, err_msg=err_msg,
--> 843                          verbose=verbose, header='Arrays are not equal')
    844
    845

XXX/numpy/testing/utils.pyc in assert_array_compare(comparison, x, y, err_msg, verbose, header, precision, equal_nan, equal_inf)
    740                 if any(x_isinf) or any(y_isinf):
    741                     # Check +inf and -inf separately, since they are different
--> 742                     chk_same_position(x == +inf, y == +inf, hasval='+inf')
    743                     chk_same_position(x == -inf, y == -inf, hasval='-inf')
    744                     x = x[~x_isinf]

XXX/numpy/testing/utils.pyc in chk_same_position(x_id, y_id, hasval)
    711                                 % (hasval), verbose=verbose, header=header,
    712                                 names=('x', 'y'), precision=precision)
--> 713             raise AssertionError(msg)
    714
    715     try:

AssertionError:
Arrays are not equal

x and y +inf location mismatch:
 x: matrix([[ inf]])
 y: array([ inf])

I guess the reason is, that a matrix stays two dimensional even after removing the nan's and then when comparing the positions of the inf's, assert_array_equal is called and that calls assert_array_compare which first checks, that the shapes of the arrays match and that then fails.

I'll have a look...

Indeed, it was a problem with slicing and matrix keeping 2 dimensions. Possibly assert_array_equal should broadcast rather than strictly check the array shape, but that presumably is water under the bridge. Anyway, a fix in #8468 (with the above example as a regression check).

Maybe I'm missing something, but why does assert_array_equal even work for duck-array types that are not ndarray subclasses? I guess we are stuck with it because scipy.sparse uses it, but I am surprised that this function does not at least use np.asanyarray to sanitize the inputs.

Just for reference: assert_array_compare does do x = array(x, copy=False, subok=True) and same for y.