-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: Period[us] start_time off by 1 nanosecond #31475
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
Changes from 6 commits
9ae8c96
430fdcf
63cd89a
9490545
4967275
444b432
c256177
58c2af7
64e12c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ PyDateTime_IMPORT | |
from pandas._libs.tslibs.np_datetime cimport ( | ||
npy_datetimestruct, dtstruct_to_dt64, dt64_to_dtstruct, | ||
pandas_datetime_to_datetimestruct, check_dts_bounds, | ||
NPY_DATETIMEUNIT, NPY_FR_D) | ||
NPY_DATETIMEUNIT, NPY_FR_D, NPY_FR_us) | ||
|
||
cdef extern from "src/datetime/np_datetime.h": | ||
int64_t npy_datetimestruct_to_datetime(NPY_DATETIMEUNIT fr, | ||
|
@@ -1169,7 +1169,12 @@ cdef int64_t period_ordinal_to_dt64(int64_t ordinal, int freq) except? -1: | |
if ordinal == NPY_NAT: | ||
return NPY_NAT | ||
|
||
get_date_info(ordinal, freq, &dts) | ||
if freq == 11000: | ||
# Microsecond, avoid get_date_info to prevent floating point errors | ||
pandas_datetime_to_datetimestruct(ordinal, NPY_FR_us, &dts) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this path is kept, then I would do if/else here and have: L 1192, L1194 after (as they are repeated at L1197, 1198) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. k looks fine, what about higher freq than microsecond? these are ok in the get_date_info path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I've got another branch im working on to handle that; it'll end up de-duplicating some code in the process There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. kk |
||
else: | ||
get_date_info(ordinal, freq, &dts) | ||
|
||
check_dts_bounds(&dts) | ||
return dtstruct_to_dt64(&dts) | ||
|
||
|
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.
Is it possible to fix
get_date_info
instead?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.
Maybe. My first two tries didnt work.
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.
so, is it possible to use this path for all freqs?
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 not for the lower-frequency freqs because of their to_end attrs e.g. Q-DEC, Q-NOV.