BUG: Fix various problems with the np.ma.masked constant #9336

eric-wieser · 2017-06-30T21:36:52Z

It turns out that np.ma.masked is actually pretty bad at preserving identity or value

This PR attempts to fix that.

It's still possible to create duplicate MaskedConstants through `.copy()`

This seems to solve the problem everywhere. It's not clear if this works on PyPy. Fixes numpygh-9328

eric-wieser · 2017-06-30T21:39:13Z

numpy/ma/core.py

+            # everywhere else, we want to downcast to MaskedArray, to prevent a
+            # duplicate maskedconstant.
+            self.__class__ = MaskedArray
+            self.__array_finalize__(obj)


I can't see a good alternative here to assigning to __class__ - unfortunately, __array_finalize__ asks us to operate in place, rather than return a new object.

Does this work on PyPy, @mattip?

Seems to be alright on PyPy.js, with a fake inheritance of MaskedConstant -> MaskedArray -> int

With my above suggestion, I'd hope this won't be necessary any more.

Pretty sure we're stuck with this - too much of the MaskedArray code calls .view(type(self)), and we really don't want to have to special case every single one of those.

An alternative approach would be to allow __array_finalize__ to have a return value, which replaces self - but there's almost certainly code out there that returns something random because it doesn't know what its doing, so that's not safe

It would seem anything.view(MaskedConstant) is almost certainly an error.... And setting __class__ is quite awful. But I defer to your judgment - I'm on holidays and worry looking in more detail is not going to be a short process...

(and I agree we cannot change __array_finalize__)

It would seem anything.view(MaskedConstant) is almost certainly an error

Written that way, yes. The problem is any kind of subclass-capable code that tries to reattach the subclass, perhaps after allocating a new buffer. That shouldn't have to know about np.ma, so np.ma needs to be able to make .view(MaskedConstant) do something sensible

eric-wieser · 2017-06-30T21:39:34Z

numpy/ma/tests/test_core.py

+        MC = np.ma.core.MaskedConstant
+        assert_(not isinstance(np.ma.masked[...], MC))
+        assert_(not isinstance(np.ma.masked.copy(), MC))
+


All of these tests previously failed

charris · 2017-07-16T16:16:48Z

@mhvk @ahaldane Could you folks take a look at this?

mhvk

I have to admit I looked at this without really referring to the rest of the code, which means I didn't understand some things. Apologies! I think my suggestion for __new__ and __array_finalize__ would be an improvement, though.

mhvk · 2017-07-16T20:14:08Z

numpy/ma/core.py

-    _mask = mask = np.array(True)
-    _baseclass = ndarray
+
+    __singleton = None


Why not just a single _?

So as not to clash with any subclasses of MaskConstant, perhaps? Why not __?

__ to me is associated with dunder while this is really just a private attribute.

__ is a special mechanism in python to do name-mangling, to make attributes unique to their class, so that property names don't clash with subclasses. __foo in the class suite of Bar is mangled to _Bar__foo

Ah, learned something new! So, here it makes sense (but for __inop it then does not; presumably that should be inherited - not that anyone really should want to inherit MaskedConstant...)

Heck, I should probably add a del __inop there anyway, which makes the name irrelevant

mhvk · 2017-07-16T20:14:35Z

numpy/ma/core.py

-    _mask = mask = np.array(True)
-    _baseclass = ndarray
+
+    __singleton = None

    def __new__(self):


While you're at it, could you change self to cls here - this really is super confusing.

mhvk · 2017-07-16T20:15:21Z

numpy/ma/core.py

+            mask.flags.writeable = False
+
+            masked = MaskedArray(data, mask=mask)
+            self.__singleton = masked.view(self)


This especially would be nicer as cls._singleton = ...

mhvk · 2017-07-16T20:22:30Z

numpy/ma/core.py

+            data.flags.writeable = False
+            mask.flags.writeable = False
+
+            masked = MaskedArray(data, mask=mask)


Here, I'd avoid the view casing and do instead (see comment on __array_finalize__):

masked = super(MaskedConstant, cls).__new__(cls, data, mask=mask)

We need the view casing anyway, because MaskedArray.__new__ calls .view

mhvk · 2017-07-16T20:23:53Z

numpy/ma/core.py


    def __array_finalize__(self, obj):
-        return
+        if self.__singleton is None:


Wouldn't it make more sense to choose path based on obj? If one uses super in __new__ as suggested above, then the relevant case that is OK is obj is None - all other cases imply view casting or slicing, neither of which I think should be allowed (except if obj is masked) - could they just raise?

mhvk · 2017-07-16T20:25:23Z

numpy/ma/core.py


    def __reduce__(self):
        """Override of MaskedArray's __reduce__.
        """
        return (self.__class__, ())

+    # inplace operations have no effect. We have to override them to avoid
+    # trying to modify the readonly data and mask arrays
+    def __inop(self, other):


Single _?

Guess I named this to look like __iadd__ without actually looking like a special method. Could call it _handle__iop__?

Would prefer that, but _iop would also be fine

Actually calling it __iop__, so that np.ma.masked.__add__.__name__ prints something somewhat reasonable

mhvk · 2017-07-16T20:35:32Z

numpy/ma/core.py

+        if self is masked:
+            return 'masked'
+        else:
+            # something is wrong, make it obvious


Just to be sure, this will not happen in any normal usage, correct? (e.g., from the test below i assume .copy returns something else)

Well, that's the hope. There were many different ways that this could happen before, and this change made it easy to spot.

It also means that subclasses of MaskedConstant don't masquerade as masked either.

OK, makes sense.

mhvk · 2017-07-16T20:36:11Z

numpy/ma/core.py

+            # everywhere else, we want to downcast to MaskedArray, to prevent a
+            # duplicate maskedconstant.
+            self.__class__ = MaskedArray
+            self.__array_finalize__(obj)


With my above suggestion, I'd hope this won't be necessary any more.

mhvk · 2017-07-16T20:37:16Z

numpy/ma/tests/test_core.py

+    def test_no_copies(self):
+        MC = np.ma.core.MaskedConstant
+        assert_(not isinstance(np.ma.masked[...], MC))
+        assert_(not isinstance(np.ma.masked.copy(), MC))


Can we add a test that checks the actual class that is returned by [...] and .copy()?

Are you worried the type might be ndarray here?

It is more that I had no idea what it would be! (As mentioned, I didn't look at the rest of the code, but from this PR it was not obvious what the result should be, and on current master it is a MaskedConstant)

Will adjust to asserting the direct type here

Fun fact - copying 0d masked arrays doesn't copy the mask

mhvk · 2017-07-16T20:38:25Z

numpy/ma/tests/test_core.py

+        assert_(not isinstance(np.ma.masked.copy(), MC))
+
+    def test_immutable(self):
+        assert_raises(ValueError, operator.setitem, np.ma.masked.data, (), 1)


Also add the test for just trying to set np.ma.masked itself?

eric-wieser · 2017-07-16T22:22:12Z

Hopefully addressed most of the reviews. This raised #9430, but I'd rather open that can of worms in another PR.

eric-wieser · 2017-07-16T23:00:54Z

numpy/ma/tests/test_core.py

+
+        m_copy = np.ma.masked.copy()
+        assert_equal(type(m_copy), np.ma.MaskedArray)
+        # assert_equal(m_copy.mask, True)  - gh-9430


This can of worms left unopened

Fixed here: #9432

charris · 2017-07-18T12:56:20Z

numpy/ma/core.py

+    # trying to modify the readonly data and mask arrays
+    def __iop__(self, other):
+        return self
+    __iadd__ = \


So this is OK because the singleton can never be unmasked? Might be cleaner stylewise to just assign the function rather than have all the line continuations.

Correct, if the singleton is unmasked there are much bigger problems.

I think I prefer the continuations, as they ensure that __iop__ doesn't get mispelt as one of the other methods

charris · 2017-07-18T13:01:53Z

numpy/ma/tests/test_core.py

+
+    def test_write_to_copy(self):
+        # gh-9328
+        x = np.ma.masked.copy()


So x is a downcast masked array, not the singleton in this case?

If so, strange behavior for copy.

Correct, because that seemed the least bad option, with the others being:

return self - this is clearly not a copy, and will fail when trying to write to it, whereas normally you would expect to be able to write to the result of copy() without side-effects

return MaskedConstant(self) (current master) - this is super confusing, because it printed as masked, and would be still be unwriteable, yet fails is masked

stop returning masked anywhere internally - this is huge compatibility change, and breaks any is np.ma.masked comparison

I take it back, this should return self, because x.copy() is x is already true for np.True_, so there's precedent for that behaviour for scalars

charris · 2017-07-18T14:23:36Z

LGTM. Do we need a release note? The behavior of masked is changed in subtle ways.

eric-wieser · 2017-07-18T14:43:49Z

Possibly an improvements release note?

eric-wieser · 2017-07-18T14:50:31Z

numpy/ma/tests/test_core.py

+
+    def test_immutable(self):
+        orig = np.ma.masked
+        assert_raises(np.ma.core.MaskError, operator.setitem, orig, (), 1)


It's a bit weird how this raises a different error, but shouldn't be a big deal

(and importantly, hasn't changed in this PR)

charris · 2017-07-18T15:27:45Z

If the behavior has changed, it should probably be in the compatibility notes.

charris · 2017-07-21T01:58:59Z

@eric-wieser Could you add a release note so I can merge this?

eric-wieser · 2017-08-29T06:40:40Z

Looks like I forgot to push the release notes fix. I'm still away from git for the next day or two

ahaldane · 2017-09-04T05:49:57Z

Code LGTM.

The following might be out of the scope of this PR, but I came across them while testing:
(edited since first posting)

The singleton's shape can be edited:

>>> np.ma.masked.shape = (1,1,1) 
>>> np.ma.array([1], mask=True)[0].shape
(1, 1, 1)

Not sure if this prexisting problem is with mean or with MaskedConstant:

>>> np.ma.masked.mean()  # fails, unlike other 0d masked arrays
AttributeError: 'int' object has no attribute 'shape'

eric-wieser · 2017-09-05T08:53:44Z

@charris - a summary of the behaviour changes:

masked is better at being the singleton it is documented to be, by not allowing copies:
- round-tripped through pickle by identity, rather than making a copy
- masked.copy() doesn't copy, because it doesn't need to.
  These seem like bugfixes that aren't note-worthy
The buffers owned by masked are now readonly
np.ma.masked.view(type(np.ma.masked)) no longer returns np.ma.masked. There are competing semantics here
- view on an array should always return a new wrapper of the same type around the same buffer
- view on a scalar should unpack the result, which in this case gives masked:
  - np.True_.view(np.ndarray) is np.True_ (identity holds for `bool_ only - otherwise it's equality)
  - np.True_.view(np.matrix) gives np.matrix([[True]], dtype=bool)
It's hard to pick something consistent here anyway, because of np.ma.masked is not a scalar #7588.

Perhaps these latter two deserve a release note.

eric-wieser · 2017-09-13T08:04:22Z

Added a release note about point 2, @charris

charris · 2017-09-21T18:42:54Z

Thanks Eric.

mhvk · 2017-10-02T17:18:16Z

That np.ma.masked is no longer hashable is breaking tests in astropy - astropy/astropy#6645 (I think we had discussion about this, but am not quite sure where...)

eric-wieser · 2017-10-02T17:20:09Z

Did this patch change whether masked could be hashed?

mhvk · 2017-10-02T17:29:17Z

Hmm, I'm just going by @astrofrog's comment that this PR broke our tests, and recalled this hashable question about MaskedConstant. Perhaps I jumped to conclusions too quickly (am in the middle of classes and meetings with students, so cannot investigate right now).

eric-wieser added 3 commits June 30, 2017 21:58

ENH: Make duplicated masked constants obvious

7254888

BUG: np.ma.masked does not preserve identity through pickle

a532242

It's still possible to create duplicate MaskedConstants through `.copy()`

BUG: Prevent copies of np.ma.MaskedConstant from being created by .view.

4ff8d0a

This seems to solve the problem everywhere. It's not clear if this works on PyPy. Fixes numpygh-9328

eric-wieser added 00 - Bug component: numpy.ma masked arrays labels Jun 30, 2017

eric-wieser commented Jun 30, 2017

View reviewed changes

BUG: np.ma.masked is mutable

8349967

eric-wieser requested review from mhvk and ahaldane July 16, 2017 16:56

mhvk reviewed Jul 16, 2017

View reviewed changes

MAINT: Improve comments and tests, in response to review

de29dc5

eric-wieser commented Jul 16, 2017

View reviewed changes

charris reviewed Jul 18, 2017

View reviewed changes

BUG: change fix for numpygh-9328 to make masked.copy() a noop

36f481b

eric-wieser force-pushed the masked-constant branch from e87fd89 to 36f481b Compare July 18, 2017 14:45

TST: Add test for direct writing of masked

2ecd05f

eric-wieser commented Jul 18, 2017

View reviewed changes

eric-wieser mentioned this pull request Aug 28, 2017

MaskedConstant should probably set its underlying arrays flags to not-writable #9594

Closed

ahaldane approved these changes Sep 4, 2017

View reviewed changes

charris added this to the 1.14.0 release milestone Sep 4, 2017

eric-wieser force-pushed the masked-constant branch from a52db41 to 1616b02 Compare September 13, 2017 08:01

DOC: Add release notes warning about np.ma.masked immutability

d4509fc

eric-wieser force-pushed the masked-constant branch from 1616b02 to d4509fc Compare September 13, 2017 08:03

charris merged commit 1ccfa62 into numpy:master Sep 21, 2017

astrofrog mentioned this pull request Oct 2, 2017

Writing simple ASCII tables broken with Numpy dev astropy/astropy#6645

Closed

eric-wieser mentioned this pull request Oct 4, 2017

BUG: Allow subclasses of MaskedConstant to behave as unique singletons #9822

Merged

Uh oh!

BUG: Fix various problems with the np.ma.masked constant #9336

BUG: Fix various problems with the np.ma.masked constant #9336

Uh oh!

Conversation

eric-wieser commented Jun 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Jul 16, 2017

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-wieser Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

eric-wieser commented Jun 30, 2017 •

edited

Loading

eric-wieser Jul 16, 2017 •

edited

Loading

eric-wieser Jul 16, 2017 •

edited

Loading

eric-wieser Jul 16, 2017 •

edited

Loading

eric-wieser Jul 18, 2017 •

edited

Loading