Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork11.8k
BUG: Fix metadata not roundtripping when pickling datetime#29555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
MaanasArora commentedAug 13, 2025
Added test! Confirmed that it fails on main. |
| forprotoinrange(2,pickle.HIGHEST_PROTOCOL+1): | ||
| res=pickle.loads(pickle.dumps(dt,protocol=proto)) | ||
| assert_equal(res,dt) | ||
| assertres.metadataisNone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That seems nice, applied, thanks!
seberg left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The None reference is a bug but we can also just apply a suggestion even. Tylers test suggestion seems like a nice to have.
| PyTuple_SET_ITEM(ret,0,PyDict_New()); | ||
| } | ||
| else { | ||
| PyTuple_SET_ITEM(ret,0,Py_None); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This steals a reference to None! Otherwise LGTM, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yes, sorry - fixed. Thanks!
| charendian; | ||
| PyObject*endian_obj; | ||
| PyObject*subarray,*fields,*names=NULL,*metadata=NULL; | ||
| PyObject*old_metadata,*new_metadata; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would probably move these down, but doesn't matter (and yeah, everything else is here so).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes, they seem more local state - I moved them down to be clearer.
seberg commentedAug 14, 2025
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. |
87e208c intonumpy:mainUh oh!
There was an error while loading.Please reload this page.
* 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)
Uh oh!
There was an error while loading.Please reload this page.
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!