BUG: Correctly distinguish between 0d arrays and scalars in MaskedArray.__getitem__ - #8905

In an ideal world, we would call getmaskarray(), and do the same as the above code path. Unfortunately, this would add a hefty performance cost for the most common cases.

#5962. This elif could be removed to fix #8906, but that might have backwards-compatibility implications

So, the test failure can be avoided by having here

elif self.dtype.type is np.object_ and dout is not masked and isinstance(dout, np.ndarray):

(And, yes, having a different .data property is more than a little annoying. But it probably was a logical choice for the original Column class, and then what do you do for MaskedColumn...)

Overall, this is probably yet another argument to not have a special case for object arrays holding ndarray and just return masked...

Do note, that #5962 didn't quite introduce this -- it just ensured that the pre-existing behaviour continued...

I still don't see where the MaskedConstant is being __new__'d from, causing the problem you described below

Oh wait, yes I do - this is #8679 rearing its head. np.ma.array(np.ma.masked, copy=False) creates a new masked constant!

Ah, yes, that closes the loop...

Added a commit to fix that - but I need your fix as well

This PR breaks a test in astropy in a very strange way, in that what is returned here fails a test where masked_column[0] is np.ma.masked (even though both seem to be masked). I'm utterly confused and this may just be a build issue, but I don't get the same problem with current master.

Can you construct a minimal example, or at least point me to that test?

@eric-wieser - it is this test
A more minimal example:

from astropy.table import MaskedColumn
import numpy as np
ma = MaskedColumn([1], dtype='O')
ma.mask = True
ma[0]
# masked
ma[0] is np.ma.masked
# False
type(ma[0]) is type(np.ma.masked)
# True

Since we overwrite __getitem__ with a CPython shim (for speed with ndarray, I also checked that:

np.ma.MaskedArray.__getitem__(ma, 0) is np.ma.masked
# False

But it has something to do with our subclass:

np.ma.MaskedArray.__getitem__(ma.view(np.ma.MaskedArray), 0) is np.ma.masked
# True

For completeness, I added just before this return in core.py:

print(mout, dout, masked is np.ma.masked)

and the output always is True 1 True.

Presumably, np.ma.MaskedArray.__getitem__(ma, 0) still looks like ma.masked?

dout is masked, or dout looks like masked but upon closer inspection is not?

Would using _data instead of data work, or would that break other bits of MaskedColumn?

Yes, using _data would work -- that is not used in MaskedColumn. But it needs some thought whether that is the right approach, i.e., to what extent one wants subclasses to be able to override how data is interpreted. Though on the other hand arguably this is so integral to how MaskedArray works that it would be reasonable to use a private property, which indicates more clearly that "if you override this, expect breakage".

Either way, I managed to reproduce this problem without touching the data attribute at all, so I'm inclined to leave it as is for now (see the added tests)

Agreed.

The behaviour here hasn't changed, the test was just less strict than it should be

I get so confused about this: why exactly is a view taken here? For isinstance(data, cls) to hold, data must be a subclass of ndarray as well, and if subok was true, then the above assignment of _data = np.array(data, subok=sub) would already have ensured that type(_data) is type(data) (independently of the other arguments). Am I missing something? If so, do add that as a comment....

I think the reason might be to ensure that a copy of the wrapper is made - as otherwise we'd modify properties of the input array. isinstance(data, cls) is checking for a MaskedArray subclass.

All I did here was swap the order of the if in order to make it easier to see what is happening.

OK, I think that makes sense. If you do have another round, do add a comment...

To me, it is not all that helpful to have these as helper functions, and I wonder a bit about the additional hit in performance.

I think the other one has to be a helper function in order for the flow control to work. I made this one a helper function because we need it twice, and it makes the lower-down code generally nicer.

I could move these to be free functions, if you think they'd be better outside the method

I'm heavily biased by having used in-method functions only in cases where I want access to some of the local variables; as this is not the case here, moving it outside might be better indeed. But this is not a strong preference, so don't worry about -- my main preference would be not to have them at all, but I can see that the code flow would be a bit odd (probably would need to start by assigning scalar_expected = None, and then at the end check whether it has change; not great either).

The only concern with moving it outside, is that it becomes distant compared to the rest of the logic. On the other hand, there's a good chance there's a bug waiting in __setitem__ that will need the same code

in cases where I want access to some of the local variables

Note that the original implementation did exactly that, but I realized it was pretty trivial to convert them into parameters, as there were only two

Maybe best to leave as is -- this is substantially clearer than it was!

Leaving it as is sounds good to me!

I think I've addressed your other concerns too - and I reckon we leave any kind of rollback of #5962 to another PR, since it's not needed to fix the bug.

Perhaps also check equality of _data and _mask as done for element 0 below?

I'll wrap this up in a helper to avoid the code duplication

Doing so

I hate giving pep8 comments, but no need for the extra indent here (only for if clauses is it needed, since if ( takes 4 characters...)

As in, you want columnar alignment here?

yes, with self and isinstance aligned; that way it is clearest what the parentheses gather together.

Yeah, I wasn't quite sure whether alignment or indenting was right here. Does PEP8 actually specify to wrap if differently to elif?

Either way, fixed, since you seem to think this is wrong, and I knew that I was unsure

The alignment is standard, with the problem of if ( being 4 characters very specifically discussed - https://www.python.org/dev/peps/pep-0008/#indentation

Of course, my real "knowledge" just comes from using emacs with flymake -- see http://docs.astropy.org/en/latest/development/codeguide_emacs.html -- which handily marks things that don't follow style, or uninitialized/unused variables, etc.