Fix MaskedNDArray mask preservation during pickling#19717
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
mhvk
left a comment
There was a problem hiding this comment.
Thanks for the PR! But I worry about problems with previously pickled instances, which were brought up by @evanshinechen in #19154 (comment) (see also my in-line comment).
My sense would be that perhaps we should follow Quantity.__reduce__ and just add __dict__ to the state. However, we then also have to make sure Quantity doesn't do the same. It is a bit tricky!
Putting "request changes" mostly since we need to be sure we do this right...
| assert type(m) is type(m2) | ||
| assert type(m.info) is type(m2.info) | ||
|
|
||
| def test_masked_ndarray_pickle_preserves_mask(): |
There was a problem hiding this comment.
Would it be possible to try to combine this test with the preceding one, by using a data entry of [1, 2, 3] in the parametrization? The only issue that I can see is that the other instances all have units, so you'd have to special-case, like
if hasattr(m, "unit"):
assert m.unit == m2.unit
| Longitude([1, 2, 3], u.deg), | ||
| ], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Don't add an empty line here.
| return (pickled_state[0], pickled_state[1], new_state) | ||
|
|
||
| def __setstate__(self, state): | ||
| super().__setstate__(state[:-1]) |
There was a problem hiding this comment.
Here we need to ensure that if something was pickled before this PR, it would be read in correctly. See discussion at #19154 (comment)
Fixes an issue where MaskedNDArray objects lost their mask after being pickled and unpickled.
This PR:
adds a regression test for MaskedNDArray pickle round-tripping
preserves _mask state during pickling via reduce
restores the mask during unpickling via setstate
Closes #19154