LGTM.

Needs updating for the recent commits. I'm not sure why the merge button is still green.

@charris - I rebased, and it passes the tests. OK to merge?

LGTM. I cannot figure out why ndarray.__getitem__ and friends were use instead of the usual indexing. Any ideas? The ma code probably dates back to the days of python 2.0, but I don't think that should make a difference.

No ideas, I'm afraid... But it seemed good to clean up, hence also #4617.

I went ahead and copied test_indexing in test_core.py to a new test_matrix_indexing. These tests fail with the current master, but with the addition of a mask.shape = dout.shape, they pass with this PR. So,it looks like this might solve even the masked matrix indexing problem! (well, better first see what travis thinks of it...).

@mhvk I'm going to delay the rest of the masked array work until 1.9 is branched.

@charris - yes, that is better, certainly for this part.

@mhvk OK, I'm back and sinking in this quicksand ;) I would help if I had a better understanding of what quantities is doing. That is, what methods are overridden, etc. If I understand correctly, the main problem here is the scalar returned by indexing. This is likely to be a general property, although ufuncs behave correctly.

In [6]: class Foo(ndarray):
    def __new__(cls, *args, **kwargs):
        return array(*args, **kwargs).view(cls)
   ...:     

In [7]: a = Foo([1]*3)

In [8]: a[...]
Out[8]: Foo([1, 1, 1])

In [9]: a[:2]
Out[9]: Foo([1, 1])

In [10]: a[1]
Out[10]: 1

In [11]: a.sum()
Out[11]: Foo(3)

Maybe there is a deeper problem here that needs to be addressed.

@charris - there are problems with both __getitem__ and __setitem__ that this PR tries to address. The one with __setitem__ is most obvious: if an item is unsuitable to be stored in the baseclass (in our case, e.g., a quantity with the wrong unit), the baseclass should be allowed to raise an exception (currently, I cannot easily give an example with quantity, since it requires some of the other PRs, but see the second half of test_subclass_items).

For __getitem__, you are indeed right that in Quantity -- and in the test case in this PR -- we are working around what one might well consider a deeper problem, that indexing a single item does not preserve subclass. But even so, by analogy with __setitem__, it would seem reasonable to just use the getter of baseclass rather than that of ndarray. (I really have no idea why MaskedArray was written to explicitly use ndarray.__setitem__ and ndarray.__getitem__.)

From a broader perspective, it would seem that MaskedArray effectively is a container class. As such, it would seem to make sense to let it make minimal assumptions about what it contains.

p.s. I rebased, which meant moving the note about this change from 1.9 to 1.10.

@mhvk OK, I'm ready to walk into this swamp again ;) Could you rebase?

@charris - OK, rebased. Haven't looked at this myself for the past year, but still think the essence here is logical: let the baseclass handle things were appropriate.

@mhvk I noticed some similar problems when combining ma and matrices, maybe you have an idea. Consider

>>> ax = matrix(np.random.rand(3,3))
>>> mx = np.ma.array(ax, mask=(np.random.rand(3,3)) < 0.5))

If you do ax*ax it performs a dot product, but doing mx*mx semi-unexpectedly performs a simple multiply.

Now, much like the changes in this PR, we could try to fix it by making MaskedArray.__mul__ end up calling self.data.__mul__ rather than (as currently) np.multiply(self.data. But my question is, how would we know that the mask should also be updated according to a dot product rather than multiply?

Similarly, a subtype might make certain ndarray methods return the elements in a different order, or in a different shape. How would np.ma know how to update the mask in those cases? (This is already sort of a problem with matrices which sometimes add dimensions of size 1, which do not get added to the mask, as I noticed here).

These problems make it seem very tough to me to make masked arrays interoperate well with arbitrary ndarray subtypes.

@ahaldane - when I wrote this PR I was mercilessly unaware of the pitfalls of matrices! Your example indeed shows a big one: either the masked version operates differently from the unmasked one, or the mask gets out of whack.

I think there are two parts to a possible solution. First would be for MaskedArray not to define __mul__, etc., but purely work at the ufunc level (i.e., with __array_prepare__, __array_wrap__, or, better, __numpy_ufunc__). Second to ensure that it takes on the methods what is being masked, so that the methods of the enclosed class are available. Then, when one calls mx * mx, this leads to Matrix.__mul__, which calls np.dot, which will first be passed through MaskedArray.__numpy_ufunc__, and there the mask can be dealt with, with the proper ufunc in hand.

But I'm far from sure this would work for all cases; in end, my sense would be that in terms of broadcasting, etc., MaskedArray should simply be able to assume that a subclass behaves like an ndarray. But obviously it does not currently, so this would break backwards compatibility.

Now on your specific example, I should add that my impression was that the matrix class has become such a problem that most people simply do not use it. Indeed, in python3.5 a new operator @ and associated __matmul__ method will be specifically introduced to make matrix operations with regular arrays easier (see http://legacy.python.org/dev/peps/pep-0465/).

@mhvk thanks for the helpful explanations. I want to better understand what the "end goal" might be for masked arrays, before I start making smaller changes.

In the case of dot, there are ways I see that almost work through varous combinations of views, but I still haven't found one that really works. The problem with your suggestion is that dot (and sum and others) are not ufuncs, and that matrix.__mul__ never goes through the ufunc machnery. A solution for dot is not going to involve ufuncs at all, I think. Also, even if matrix multiplication is fixed up in python3.5, there is still a similar problem with sum and others. Anyway, I'll keep thinking about it.

@mhvk Could you rebase this again. It's probably just the release notes causing problems.

@charris - ok, rebased. As you guessed, it was just the release notes -- and travis still passes!

I've convinced myself that the gain is worth the possible pain, so in it goes.

Thanks @mhvk .