REV: Add MaskedArray creation from non nd-array back in #20386
Conversation
numpy/ma/core.py
Outdated
| # still has the _mask attribute like MaskedArrays | ||
| if hasattr(data, '_mask') and not isinstance(data, ndarray): | ||
| _data._mask = data._mask | ||
| _data._sharedmask = True |
There was a problem hiding this comment.
Was that actually there before, I think this is a bug-fix probably? I feel there was some discussion about it being _sharedmask = True rather than _data._sharedmask = True?
There was a problem hiding this comment.
Here is the discussion thread: #19102 (comment)
with the previous code removed from that commit
# FIXME: should we set `_data._sharedmask = True`?
# Previously this set `_sharedmask = True`, but
# that didn't actually do anything as the variable was unused!I think this makes sense from the standpoint of when we are putting the incoming data._mask onto the new object, we should inform the original object that it is now sharing its mask with something else. I agree that it is not a full reversion in that sense, but I wasn't sure whether to call this a fix or revert PR, since the revert part is probably more important to get across. Happy to remove this and go back to a pure revert, or I can also add this "fix" piece as a separate commit if that would be more clear too.
There was a problem hiding this comment.
I am good with this (might rename the PR title, but that is all). I just wanted to make sure that you are certain that it is correct (if we could add a test for it that would be awesome).
Basically, I am not deep into masked arrays, so just probing to build my trust in the fix ;).
There was a problem hiding this comment.
OK, good suggestion to dig into this a bit more @seberg :)
Adding _data.sharedmask = True would actually break the case where keep_mask=False and overrides the mask later, but doesn't update the _sharedmask attribute. I decided to leave this as purely a partial revert of that commit and added a new test for that case I found at the end here as well.
Another option is to fix this by adding in the _data._sharedmask = True here and forcing sharedmask = False in the if statement here:
Line 2847 in f146ec1
There was a problem hiding this comment.
But, I think that should be for a separate FIX PR if someone thinks it is necessary/desirable.
Sounds good. There are not many actively invested in masked arrays. So if you think it is desirable than that is definitely "someone" :).
This code path was removed in beacb39. This adds back in the MaskedArray portion of that commit. A test with a Quantity-like (non inherited, but still acts like a MaskedArray) class for this case.
9d96222 to
90ca9be
Compare
|
Thanks for the quick fix @greglucas! If you feel like following up, please do. We would only put that in after branching 1.22.x anyway, so it would sit on the development/main branch for a while.
Just going to squash-merge, no need. |
This code path was removed in beacb39. This adds back in the MaskedArray portion of that commit
and also fixes the _shared_mask attribute of the new MaskedArray. A test with a Quantity-like (non inherited, but still acts like a MaskedArray) class for this case.closes #20372