BUG: Ensure __array_finalize__ cannot back-mangle shape#10314
Conversation
There was a problem hiding this comment.
Might be clearer to add dtype and shape @properties that fall back on ndarray.shape.__get__ etc
There was a problem hiding this comment.
Hmmm... I'd actually expect the stronger result of y1a is y1 here
There was a problem hiding this comment.
Maybe add a comment referencing that issue here?
There was a problem hiding this comment.
Wait, what? This doesn't seem sensible to me at all (#10318)
|
@eric-wieser - I agree the changes in behaviour for Also agreed that setters for |
mhvk
force-pushed
the
ma-array-finalize-mask-view
branch
2 times, most recently
from
e39107a to
97628c2
Compare
|
@eric-wieser - your comment reminded me about the existence of this branch... I now added a second commit replacing I also still need to think of a test case that shows this actually prevents any problems! |
There was a problem hiding this comment.
I think this should be just super(MaskedArray).shape or perhaps just ndarray.shape
There was a problem hiding this comment.
Edit: I'm wrong. TIL how super(cls, cls) works
There was a problem hiding this comment.
Yes, this annoyance is in part why I wondered if it was a good idea to replace __setattr__ with this...
There was a problem hiding this comment.
The advantage is that subclasses are now able to call Maskedarray.shape.__set__ in the unlikely even they override shape, and it won't accidentally call ndarray.shape.__set__
There was a problem hiding this comment.
Actually, I think this should be super(MaskedArray, type(self)), which correctly walks the MRO.
|
Is this enough for a test? On >>> a = np.ma.array([1, 2, 3, 4], mask=True)
>>> d = a.data
>>> m = a.mask
>>> a.shape = (2, 2)
>>> d
array([1, 2, 3, 4])
>>> m # your patch stops this changing shape, which is consistent with d
array([[ True, True],
[ True, True]], dtype=bool) |
|
Hmm, that particular case is not solved by this PR - it needs your original one, where the mask is always viewed. Indeed, I found it very hard to far to get to a case where my PR matters... I'll try a bit more, but if it fails, perhaps best to split it into two - and at least get rid of the |
|
Ok, we now have a concrete problem caused by this, as documented in #3140 |
Since dtype and shape are properties, this needs a somewhat ugly super construction; see https://bugs.python.org/issue14965
mhvk
force-pushed
the
ma-array-finalize-mask-view
branch
from
97628c2 to
b779bae
Compare
|
Thanks for realizing there was a test case after all! I now added it, and rebased for good measure (and fixed the |
|
|
||
| @property | ||
| def shape(self): | ||
| return super(MaskedArray, self).shape |
There was a problem hiding this comment.
Pretty sure this needs to be super(MaskedArray, type(self)).shape.__get__(self, type(self)), unless there's something weird about base classes in C
There was a problem hiding this comment.
Ok, seems there's something weird about base classes. If this were shadowing another property (rather than a getset_descriptor) , you'd need what I have above.
There was a problem hiding this comment.
I don't think that's true - I;m fairly sure one can shadow a property getter as I have it above, just not the setter.
charris
changed the title
__array_finalize__ cannot back-mangle shape
2 participants
Just a small step, but
__array_finalize__should not be allowed to change the shape of anyobjpassed in. Arguably, it should not change the shape of the mask at all, but removing that currently breaks quite a bit more, so perhaps it is best to try to remedy that in a second step.Need to think a bit about a test case that shows that this actually prevents problems.