-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG/API: preserve non-nano in factorize/unique #51978
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Looks good to be honest, I don't think I'd consider the cache vs non-cache discrepancy a blocker
msg = "Out of bounds nanosecond timestamp: 9999-01-01 00:00:00" | ||
with pytest.raises(OutOfBoundsDatetime, match=msg): | ||
to_datetime(dts_with_oob, errors="raise") | ||
# As of GH#?? we do not raise in this case |
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.
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.
have added this to my next CLN branch
Fine with me to have this fix despite the cache=False bug. Could you add a test for the above too? |
Thanks @jbrockmendel |
Owee, I'm MrMeeseeks, Look at me. There seem to be a conflict, please backport manually. Here are approximate instructions:
And apply the correct labels and milestones. Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon! Remember to remove the If these instructions are inaccurate, feel free to suggest an improvement. |
Mind creating the backport PR @jbrockmendel ? |
* BUG/API: preserve non-nano in factorize/unique * test (cherry picked from commit 6a13450)
timedelta64[s]
to timedelta64[ns] #51961 (Replace xxxx with the GitHub issue number)doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.The ugly bit here is that it causes
to_datetime
to behave slightly differently with cache=True/False in some cases. I know @mroeschke worked to avoid that in #17077, so it may be a deal-breaker.But it fixes #51961 (though doesn't yet have a targeted test) so may be worth prioritizing (cc @MarcoGorelli)
(Update) also fixes Series[non-nano].factorize which currently raises AssertionError