BUG: Fix metadata not roundtripping when pickling datetime #29555
Conversation
|
Added test! Confirmed that it fails on main. |
| for proto in range(2, pickle.HIGHEST_PROTOCOL + 1): | ||
| res = pickle.loads(pickle.dumps(dt, protocol=proto)) | ||
| assert_equal(res, dt) | ||
| assert res.metadata is None |
There was a problem hiding this comment.
Since this test already has 7 other assertions above, any objection to just making it its own regression test:
--- a/numpy/_core/tests/test_datetime.py
+++ b/numpy/_core/tests/test_datetime.py
@@ -889,8 +889,8 @@ def test_pickle(self):
b"I1\nI1\nI1\ntp7\ntp8\ntp9\nb."
assert_equal(pickle.loads(pkl), np.dtype('>M8[us]'))
+ def test_gh_29555(self):
# check that dtype metadata round-trips when none
- # gh-29555
dt = np.dtype('>M8[us]')Beyond that, the regression test does indeed seem to fail before/pass after the patch in my hands locally.
There was a problem hiding this comment.
That seems nice, applied, thanks!
| PyTuple_SET_ITEM(ret, 0, PyDict_New()); | ||
| } | ||
| else { | ||
| PyTuple_SET_ITEM(ret, 0, Py_None); |
There was a problem hiding this comment.
This steals a reference to None! Otherwise LGTM, though.
There was a problem hiding this comment.
Oh yes, sorry - fixed. Thanks!
| @@ -2877,6 +2878,7 @@ arraydescr_setstate(_PyArray_LegacyDescr *self, PyObject *args) | |||
| char endian; | |||
| PyObject *endian_obj; | |||
| PyObject *subarray, *fields, *names = NULL, *metadata=NULL; | |||
| PyObject *old_metadata, *new_metadata; | |||
There was a problem hiding this comment.
Would probably move these down, but doesn't matter (and yeah, everything else is here so).
There was a problem hiding this comment.
Yes, they seem more local state - I moved them down to be clearer.
|
Thanks @MaanasArora, let's put this in. This has been around forever, but I can't really see that not including an empty metadatadict in such a niche use-case can break anything, so I think we could backport. |
* BUG: fix metadata not roundtripping for datetime pickles * STYLE: Remove unnecessary newline * REF: Simplify metadata handling in arraydescr_setstate function * TST: add test to ensure datetime dtype metadata round-trips on pickling * REF: clearer operation order * TST: add new regression test (numpygh-29555) * BUG: don't steal reference to Py_None * REF: move metadata variable declarations below for clarity
BUG: Fix metadata not roundtripping when pickling datetime (#29555)
Fixes #29344.
It seems that for datetime dtypes,
arraydescr_reducewas initializing a new mapping instead of setting toPy_None, and thenarraydescr_setstatejust read it in. I set the metadata to and then checked forPy_Noneand refactored some of the logic to be clearer hopefully.Adding a test. Thanks for reviewing!