BUG: Fix MaskedArray.__setitem__ - #8594

@eric-wieser - apart from the very minor comments, why the check for the object dtype at all? It seems this is not required to make the test case work (I do agree with the sentiment, but this is not really a function that will be used outside of np.ma -- indeed, one could also insist any dtype passed in is already a dtype instance).

Separately: nice to actually use getmask consistently!

It seems this is not required to make the test case work

Well, it is! Otherwise test_set_element_as_object fails.

Ah, I see, it was an existing test that starts breaking with your change; should have considered that possibility. But I fear this will fail the general case. E.g., what happens if one is trying to do ma['object'] = <object> if ma is a masked single-element record array and object is a record with objects?

Might it be an idea to move this object-checking piece of the logic out of getdata and in to __setitem__ (where one has self-awareness and can thus choose not to go through getdata at all if one is trying to set a single object).

But I fear this will fail the general case.

Note that it can't fail any existing case, since the only place right now that sets the dtype parameter of getdata is in __setitem__. Without that argument, the behaviour is as (broken?) as before.

I meant the "general case of __setitem__".

Indeed, that you only use the dtype in __setitem__ is why I suggested to keep the "does self hold objects" part of the logic there. As is, getdata always returns an array, and I think it makes a lot of sense to allow one to tell what dtype the result should have, but it makes less sense to allow it not to return an array any more.

Looking more at test_set_element_as_object, I realize some parts of numpy I don't like. At least, I find the following bizarre:

a = np.ma.empty(5, dtype=object)
a[0] = x
a
# masked_array(data = [(1, 2, 3, 4, 5) None None None None],
#              mask = False,
#        fill_value = ?)
a[:] = x
a
# masked_array(data = [1 2 3 4 5],
#              mask = False,
#        fill_value = ?)
a[0:1] = x
# ValueError: cannot copy sequence with size 5 to array axis with dimension 1

Anyway, it does seem we're stuck with this.

I think that's consistent with how non-ma arrays behave

p.s. Is there anything against just using dval = getattr(value, '_data', value)? This is in closer analogy to what getmask does, and may solve everything too? At least, I just tried on your branch and there are no errors...

@mhvk: Very tempting. I'd argue that getdata should become that, not just the use in setitem. Or at the very least, everything in ma.core should maybe use that.

@mhvk: Actually, that makes much more sense as an approach, as really we want to forward array-conversion behaviour on to the base ndarray functions, rather than try and emulate the same behaviour ourself in np.ma.

I'm not sure if it's acceptable to change the public behaviour of getdata in that way though, so perhaps the entire of ma would use _getdata(), and getdata can be deprecated?

@mhvk: Really good spot there - this probably caused other bugs in subtle places as well. Updated to use getattr(value, '_data', value) plus some backwards-compatible boilerplate

@eric-wieser - I tried to do something quite similar in an earlier PR, and at least at the time that was felt to be too risky. My suggestion, therefore, would be to split this PR in an uncontroversial one that fixes the bug (i.e., use getattr(value, '_data', value) in __setitem__) and a maintenance PR that updates getdata (the latter should probably also have the generic use of getmask since it is not necessary for the bug fix).

But perhaps best to ask a real maintainer...

split this PR in an uncontroversial one that fixes the bug (i.e., use getattr(value, '_data', value) in __setitem__

I suspect there are a whole class of similar bugs, that mean this change should be made through np.ma.core. But perhaps exposing this change publically is a bad idea

@eric-wieser - looking back at this (now with a maintainer hat on): I do very much like the change to getdata in (as I think it is much more logical, and means there is at least a hope of a MaskedQuantity class, one of my longstanding goals...), but it also still seems to me that that part really is not for a bug fix.

So, I think this is best done in two steps, the bug fix just using getattr(value, '_data', value), and then a MAINT PR that changes getdata. Is that OK with you?

@mhvk: I've been trying to conjure up other bugs that arise from the current implementation, but haven't found any yet. If I don't succeed in doing so, then yes, I guess I'll split the PR

Also, I moved the np.ma.where stuff to #8647, which it seems the change to getdata requires be merged first

OK; if it does stay together, then I guess it should be MAINT for 1.13 -- one cannot really introduce a deprecation warning in a bug-fix release...

Oh, I see what you mean now - I hadn't realized targeting 12.1 was on the cards. How would you feel about me introducing _get_data, and using that everywhere in ma/core.py? Then the MAINT commit can just rename that to get_data

My sense remains that we should just fix the __setitem__ bug for the bug fix, not make further changes...

And fix it for 1.12 too then?

In that case, I should rebase on the branch point for 1.12. Where is that?

Update: 1718ee8?

@mhvk: Ok, I've stripped it down to just that change, and rebased on the branch point.

OK, that looks good. I'll merge assuming the tests pass (don't see how they could not, but just to be sure).

Arguably #8648 is one that should have the 12.1 release milestone

@mhvk: tests pass :)

Hmm, yes, it is my astropy background again, where we set the milestone to the lowest relevant release and do backporting after the fact. But I think you've done the right thing here, so will merge both...

@mhvk Our backport policy is still somewhat ad hoc, exspecially as I the only one who has been doing release. What I currently do is set the milestone to the earlier version, so I will find the PR when looking for backports, then do a backport, label it as such, set the milestone on the backported version, and remove the milestone from the original. I'm not completely happy with the process, so if you have better ideas I'd like to hear them. One option I've considered is a backported label in addition to the backport label, then use the latter for things to be backported.

Thanks @mhvk. This now unblocks #8511 :)

@charris - OK, so clearly we should continue to set the milestone to a bug-release version; that is no effort to anyone and keeps things clear. For the rest, it would be nice if things could be more automated. E.g., might it be possible to have some travis magic that does a trial merge & test? But maybe we better discuss this on the mailing list? ... which I will do now.