-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Period constructor raises instead of ignoring when passing a string with extra precision(pico, femto, etc.) #50417
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
BUG: Period constructor raises instead of ignoring when passing a string with extra precision(pico, femto, etc.) #50417
Conversation
Honestly, it might be worth to kill @MarcoGorelli interested? |
I'll take a look, thanks for the ping! |
@@ -386,7 +386,7 @@ cdef parse_datetime_string_with_reso( | |||
&out_tzoffset, False | |||
) | |||
if not string_to_dts_failed: | |||
if dts.ps != 0 or out_local: | |||
if out_bestunit == NPY_DATETIMEUNIT.NPY_FR_ns or out_local: |
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.
makes sense. to be totally correct we might need to check for ps, fs, as?
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.
Good point. What should be the error message we raise, or should we silently ignore?
On 1.5.2, doing pd.Period("1970/01/01 00:00:00.000000000111")
gives me
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "pandas/_libs/tslibs/period.pyx", line 2579, in pandas._libs.tslibs.period.Period.__new__
File "pandas/_libs/tslibs/parsing.pyx", line 369, in pandas._libs.tslibs.parsing.parse_time_string
File "pandas/_libs/tslibs/parsing.pyx", line 431, in pandas._libs.tslibs.parsing.parse_datetime_string_with_reso
KeyError: 11
Timestamp seems to ignore the extra precision, so maybe we should match that here?
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.
definitely better than the status quo. it might be reasonable to have Timestamp warn/raise instead of silently truncating, but thats out of scope.
@@ -395,6 +395,12 @@ cdef parse_datetime_string_with_reso( | |||
parsed = datetime( | |||
dts.year, dts.month, dts.day, dts.hour, dts.min, dts.sec, dts.us | |||
) | |||
# Match Timestamp and drop picoseconds, femtoseconds, attoseconds | |||
# The new resolution will just be nano |
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.
GH ref pointing back here?
ping @jbrockmendel |
thanks @lithomas1 |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.