BUG: Fix broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object) #8122

raghavrv · 2016-10-05T16:17:28Z

Fixes #1940 and #8113

Is this fix correct? Should this be extended to mrecords too?

@pierregm @teoliphant

amueller · 2016-10-05T16:39:28Z

I'm pretty sure the days when @teoliphant had time to review code are over ;)

raghavrv · 2016-10-05T16:41:51Z

I randomly pinged him as I saw him in the Authors list. :p

raghavrv · 2016-10-05T16:43:07Z

Any one of @juliantaylor @rgommers or @seberg can take a look at this maybe? :)

shoyer · 2016-10-05T17:03:09Z

Is it possible to implement this with super(MaskedArray, self).__getstate__ instead? That's probably a more robust solution.

raghavrv · 2016-10-05T17:21:27Z

I thought about that but apparently the ndarray does not implement a __getstate__ method... The other option would be to use the __reduce__ and extract the state from that. But it feels hackier. Suggestions?

jnothman · 2016-10-05T23:36:21Z

numpy/ma/tests/test_core.py

+                assert_equal(a_pickled._mask, a._mask)
+                assert_equal(a_pickled._data, a._data)
+                if dtype is not object:
+                    assert_equal(a_pickled.fill_value, dtype(999))


is there a reason not to test this for object?

Thanks for the catch... I missed an else here :( Fixed it in the recent commit...

jnothman · 2016-10-05T23:37:42Z

@raghavrv I wouldn't usually ping anyone. Most projects have someone - or a rabble - to do triage on new issues/PRs.

jnothman · 2016-10-05T23:40:59Z

@shoyer wrote:

Is it possible to implement this with super(MaskedArray, self).__getstate__ instead? That's probably a more robust solution.

Would that maintain backwards compatibility like the proposed patch? At least this patch appears to be a good fix, even if a more robust solution is possible at a later point.

shoyer · 2016-10-06T01:11:51Z

The first five elements of MaskedArray.__getstate__ are exactly the state produced by nparray.__reduce__:

In [49]: x = np.ma.MaskedArray([1, 2, 3, 4], [False, True, False, False])

In [50]: x.__getstate__()
Out[50]:
(1,
 (4,),
 dtype('int64'),
 False,
 b'\x01\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00',
 b'\x00\x01\x00\x00',
 None)

In [51]: _, _, state = super(np.ma.MaskedArray, x).__reduce__()

In [52]: state
Out[52]:
(1,
 (4,),
 dtype('int64'),
 False,
 b'\x01\x00\x00\x00\x00\x00\x00\x00\x02\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00\x00\x00\x00\x00\x04\x00\x00\x00\x00\x00\x00\x00')

So I think that's what we want to use. It is a little weird to pull it out of __reduce__, but it's actually less code for MaskedArray and makes sense considering the implementation of __setstate__ (which already calls the super method).

seberg · 2016-10-06T09:25:16Z

Just to note (not sure if it has much to do), but currently it seems masked arrays do not preserve subclasses during pickling. If you use the data's reduce, I am not sure if there might be an effect.

jnothman · 2016-10-06T11:01:49Z

numpy/ma/core.py

-                 # self._data.tolist(),
-                 getmaskarray(self).tobytes(cf),
+        data_state = super(MaskedArray, self).__reduce__()[2]
+        print(data_state)


Oops sorry!!

jnothman · 2016-10-06T11:03:38Z

numpy/ma/core.py

-                 getmaskarray(self).tobytes(cf),
+        data_state = super(MaskedArray, self).__reduce__()[2]
+        print(data_state)
+        state = (data_state[0],     # version


Can you just do data_state + (getmaskarray(self).tobytes(cf), self._fill_value)?

raghavrv · 2016-10-06T11:19:07Z

@shoyer @jnothman Have addressed the comments. Let me know if anything else needs to be changed...

jnothman · 2016-10-06T11:26:34Z

numpy/ma/core.py

-                 self._fill_value,
-                 )
-        return state
+        data_state = super(MaskedArray, self).__reduce__()[2]


@shoyer, does this really make more sense given that we're making assumptions about super.__reduce__'s return value, and discarding all elements but one?

I'm pretty comfortable with this, given that (1) the spec of __reduce__ is mostly prescribed by Python and this MaskedArray is defined in NumPy itself, which means that if any ever changes how ndarray.__reduce__ works our unit tests will catch the issue for MaskedArray.

shoyer · 2016-10-06T16:31:50Z

This looks good to me. @charris any concerns? If not, let's merge (and mark this one for a potential backport, if we do another bug fix release).

charris · 2016-10-06T19:43:44Z

@shoyer I'll leave it up to you. The change seems small and nothing breaks, so probably good. I don't plan on any more bug fix releases, I'd actually like to get the first rc of 1.12.0 out this month.

charris · 2016-10-06T19:44:59Z

Note that super only works for new style classes, which may, or may not, be an issue for Python 2.7.

raghavrv · 2016-10-06T19:48:25Z

Do you want it to be ndarray.__reduce__(self) instead?

charris · 2016-10-06T19:55:55Z

@raghavrv Not to worry, looks like MaskedArray is a new style class

In [4]: str(ma.MaskedArray)
Out[4]: "<class 'numpy.ma.core.MaskedArray'>"

Note that the string begins with <class.

raghavrv · 2016-10-06T19:56:48Z

Ah. Sweet then! Thanks for the information!

shoyer · 2016-10-06T21:31:17Z

OK, in it goes. Thanks @raghavrv!

jnothman · 2016-10-06T22:18:53Z

Good work, @raghavrv.

On 7 October 2016 at 08:31, Stephan Hoyer notifications@github.com wrote:

OK, in it goes. Thanks @raghavrv https://github.com/raghavrv!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#8122 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEz6zGgpJ5uJo4uCS0e66DhVQBUPPgfks5qxWitgaJpZM4KPASm
.

raghavrv · 2016-10-06T23:41:41Z

Thanks for the reviews @jnothman @shoyer and @charris :)

raghavrv added 3 commits October 4, 2016 15:46

TST add test to check pickling of object dtyped masked array

6b0d1e3

Serialize to list instead of bytestring for the data array

3669bfa

Test for different masks

2ed5002

raghavrv mentioned this pull request Oct 5, 2016

Error unpickling RandomizedSearchCV objects in 0.18 due to masked arrays scikit-learn/scikit-learn#7562

Closed

jnothman reviewed Oct 5, 2016

View reviewed changes

Test fill value for object dtype too

66993e0

jnothman reviewed Oct 6, 2016

View reviewed changes

Use ndarray.__reduce__ to extract the bytestring/list

1fa49f8

raghavrv force-pushed the masked_array_pickle branch from b47127b to 1fa49f8 Compare October 6, 2016 11:15

jnothman reviewed Oct 6, 2016

View reviewed changes

charris changed the title ~~[MRG] FIX broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object)~~ BUG: Fix broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object) Oct 6, 2016

charris added 00 - Bug component: numpy.ma masked arrays labels Oct 6, 2016

raghavrv mentioned this pull request Oct 6, 2016

[MRG+1] FIX Make sure GridSearchCV and RandomizedSearchCV are pickle-able scikit-learn/scikit-learn#7594

Merged

shoyer merged commit 8888a76 into numpy:master Oct 6, 2016

raghavrv deleted the masked_array_pickle branch October 6, 2016 21:36

raghavrv mentioned this pull request Oct 10, 2016

Masked array with object dtype doesn't unpickle #8113

Closed

TomDLT mentioned this pull request Oct 21, 2016

Masked array with dtype=object are not pickled correctly #8191

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object) #8122

BUG: Fix broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object) #8122

raghavrv commented Oct 5, 2016 •

edited

Loading

amueller commented Oct 5, 2016

raghavrv commented Oct 5, 2016

raghavrv commented Oct 5, 2016

shoyer commented Oct 5, 2016

raghavrv commented Oct 5, 2016

jnothman Oct 5, 2016

raghavrv Oct 6, 2016

jnothman commented Oct 5, 2016

jnothman commented Oct 5, 2016

shoyer commented Oct 6, 2016

seberg commented Oct 6, 2016

jnothman Oct 6, 2016

raghavrv Oct 6, 2016

jnothman Oct 6, 2016

raghavrv Oct 6, 2016

raghavrv commented Oct 6, 2016

jnothman Oct 6, 2016

shoyer Oct 6, 2016

shoyer commented Oct 6, 2016

charris commented Oct 6, 2016

charris commented Oct 6, 2016

raghavrv commented Oct 6, 2016

charris commented Oct 6, 2016

raghavrv commented Oct 6, 2016

shoyer commented Oct 6, 2016

jnothman commented Oct 6, 2016

raghavrv commented Oct 6, 2016

BUG: Fix broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object) #8122

BUG: Fix broken pickle in MaskedArray when dtype is object (Return list instead of string when dtype is object) #8122

Conversation

raghavrv commented Oct 5, 2016 • edited Loading

amueller commented Oct 5, 2016

raghavrv commented Oct 5, 2016

raghavrv commented Oct 5, 2016

shoyer commented Oct 5, 2016

raghavrv commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Oct 5, 2016

jnothman commented Oct 5, 2016

shoyer commented Oct 6, 2016

seberg commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raghavrv commented Oct 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shoyer commented Oct 6, 2016

charris commented Oct 6, 2016

charris commented Oct 6, 2016

raghavrv commented Oct 6, 2016

charris commented Oct 6, 2016

raghavrv commented Oct 6, 2016

shoyer commented Oct 6, 2016

jnothman commented Oct 6, 2016

raghavrv commented Oct 6, 2016

raghavrv commented Oct 5, 2016 •

edited

Loading