I'm still wondering whether it would be better to use .view(type(self)) as long as that is not mvoid while the shape is that of an array.

Not sure what you mean here.

Oh, you mean in the following line?

Yes, indeed. In the old code, the result is unconditionally turned into type(self), which is a problem if one compares a MaskedArray with an mvoid (where the latter is a single element, which will always have precedence since it is a subclass of MaskedArray.

Possibly, it is better solved in mvoid...

Anyway, as written the code works, but let me know if there are other suggestions.

Yes, probably it can also be solved by defining _compare in mvoid, but I agree this is fine.

(also, not sure if the test above for check.dtype.names should be something like check.dtype == np.void to account for dtype('V4')... but maybe that is overthinking it since I suspect truly void types don't work well with masks anyway)

Could use a bit more explanation of the role of the mask.

Could use a bit more explanation of the role of the mask.

Wasn't there before, but happy to do so (but will wait for possible other comments first).

I know this is the old behavior, but just want to note that any makes more sense to me than all.

With all, I think it causes the result of the comparison to depend on the data at a masked value, eg:

>>> m = np.ma.mvoid((1,2), mask=(0,1), dtype='i4,f4')
>>> n = np.ma.mvoid((1,0), dtype='i4,f4')
>>> o = np.ma.mvoid((1,1), dtype='i4,f4')
>>> (n == m), (m == n), (o == m)
(True, masked, False)

@ahaldane To be clear, is that the behaviour you want, or the behaviour that all implies?

That's the behavior I get with this PR as it is right now (I fetchd + tested it).

I actually didn't work out why n == m is different from m ==n yet.

Should m == n return masked or True? Either seem somewhat reasonable (but obviously both as they are right now is bad!)

Now that you ask, I have been going back and forth on it...

After some thought, the only consistent result I see is to return masked for all three cases, except that we need some extra code so that m == m, ie

>>> p = np.ma.mvoid((1,7), mask=(0,1), dtype='i4,f4')
>>> (m == p), (m == m)
True, True

which I think requires replacement of mask_or by mask_xor, and then use any instead of all. But possibly we should have m == m return masked, I'm still a little undecided.

Returning True seems worse to me because intuitively I think n and m don't represent equal values, and secondly because the code would have to be much more complicated: We would have to reimplements numpy's internal structure comparison (somewhere in the C code) inside of MaskedArray to ignore fields with masked values.

@ahaldane: I'm suggesting all semantics on the flattened view of the masked data (when such a thing can exist), not on the mask alone. I'm suggesting this because that's what the semantics of normal structured arrays are.

Flattening the data in your example, we get consistent behaviour

>>> o = np.ma.array([[1]], mask=[[True]])
>>> p = np.ma.array([[0]], mask=[[True]])
>>> (o == p).all(axis=-1)
masked_array(data = [--],
             mask = [ True],
       fill_value = True)

I'd expect your example to return a 1D array, not a scalar, but I assume that's just a mistake.

I think I'm with @eric-wieser here, but maybe best to postpone further discussion until I have the next version up; @ahaldane's example shows that this one is more broken than I had thought (though an improvement on what was there!). In the meantime, I also found that other parts of np.ma are relying on comparison of two masked items being returned as masked True...

By the way, I just noticed there are unused, half-implemented methods _get_recordmask and _set_recordmask defined for MaskedArray...

Isn't np.ma lovely...

A lot of work an exploration was put into it, which hopefully we can learn from in a future tidying/reimplementation. (incidentally, if more numpy members who know about maskedarrays are joining recently, maybe we can make a reimplementation happen soon)

With respect to this PR: I recognize that, as often happens, this PR might be snowballing now that the unit tests are failing upon further changes. I might be in favor of getting this PR through now as a partial fix (at least m == m[0] will work). What do you think?

I personally think that every single one of these tests should return np.masked

In fact, can any two mvoid objects return np.masked on comparison with this patch?

My argument would be that there are 3 cases here:

• No matter what values the masked fields take, two objects are always equal
• No matter what values the masked fields take, two objects are always not equal
• The equality depends on the values of the masked fields

Since the first two of these cases are represented by True and False, surely we should use masked to represent the third case?

Yes, mvoid comparisons can yield masked, but only if all elements are masked (in at least one of the two):

m = mvoid((1, 2.), mask=(0, 1), dtype='i4,f4')
p = mvoid((1, 2.), mask=(1, 0), dtype=m.dtype)
m == p
# masked

I probably should add that to the test cases...

This is the test I had to change (as well as its equivalent for ne).

This seems inconsistent with the duck-typing we seem to use elsewhere

OK, done.

I think this might be clearer as just sbroadcast, odatabroadcast = np.broadcast_arrays(self, odata, subok=True)

Yes, on second thought, I agree clarity beats speed here (I essentially wrote out what np.broadcast_arrays does, but skipping broadcasting odata).

@mhvk: I'm also suggesting you pass odatabroadcast rather than odata into filled, for clarity

In fact, I'm starting to think that this should be the default behaviour of filled, allowing filling with a larger fill-value than the dimensions

actually, that breaks tests, it turns out: broadcast_arrays is not guaranteed to return a new instance, and since I set the mask right after, I effectively change self. Not sure that is desired behaviour, but outside of the scope of this PR...

And looking into this, I realise mask is not properly broadcast either (and never was): this fails

np.ma.array([0, 1], mask=[0, 1]) == np.ma.array([[0, 2]])

(it fails in __repr__ but would otherwise be bad too)

@eric-wieser - as you'll see, I had to leave the broadcasting alone, since otherwise I had to make a copy. I agree with you that, ideally, filled would just do the right thing itself. Even better if the filled function could take a mask rather than just calling the method. But I think that's best left for another PR!

This is probably better as if np.ndim(check) == 0

Oh, unless this is deliberately trying to return 0d arrays and not scalars

There's a function somewhere within this file for picking the appropriate subclass of MaskedArray

Are there any tests of this underlying bool data?

Yes, note how the tests all do something like test = (?? != ??) and then test both the values and the mask.

If this is trying to test the underlying values, I think it needs a .data

You're presuming that numpy functions correctly account for subclasses.... ;-)

In [2]: test = np.ma.MaskedArray([True, True], mask=[False, True])
In [3]: assert_equal(test, [True, True])
In [4]: test = np.ma.MaskedArray([True, False], mask=[False, True])
In [5]: assert_equal(test, [True, False])

(they should, of course, but that's for another time....)

Wanna bet they don't?

In [2]: test = np.ma.MaskedArray([True, True], mask=[False, True])
In [3]: assert_equal(test, [True, True])  # correct, as before
In [4]: assert_equal(test, [True, False])  # uh oh...
In [5]: assert_equal(test, [True, np.array("oh dear", dtype=object)])  # oh geez

Grrrrrr.... OK, will change.

regular would be better as un-structured here, since regular falsely implies "not masked"