-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
REGR: to_timedelta precision issues with floating data #25651
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 3 commits
4e36a9b
fb67db4
39b15aa
338a652
5cc3c39
943888b
74c3e32
053df8d
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 |
---|---|---|
|
@@ -246,6 +246,47 @@ def array_to_timedelta64(object[:] values, unit='ns', errors='raise'): | |
return iresult.base # .base to access underlying np.ndarray | ||
|
||
|
||
def precision_from_unit(object unit): | ||
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. do this instead make a new function that is cpdef that calls cast_from_unit, then you don't remove the inline nature from cast_from_unit and you can call it from python w/o re-writing. 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. So the problem is that the current I tried putting the common logic in a separate function, but it starts to become a bit more ugly to write it in such a way that it can still be used in the cdef inline one. 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 added a bit convoluted way to share the if/else part, but I don't see an easy other way to share that part, except by not having 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. you need to use cpdef. 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. Can you cpdef inline? 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. no. but inline for this prob doesn't make much difference. its the cast_from_unit where it actually matters a lot. 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. How do you do an 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. actually. you are only calling this once? then make it a def is just fine (and error checkng is easy) 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. In the new code, I am calling this only once. But the existing 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. Updated to use a cpdef as asked. It's only used in places where a similar cpdef |
||
cdef: | ||
int64_t m | ||
int p | ||
|
||
if unit == 'Y': | ||
m = 1000000000L * 31556952 | ||
p = 9 | ||
elif unit == 'M': | ||
m = 1000000000L * 2629746 | ||
p = 9 | ||
elif unit == 'W': | ||
m = 1000000000L * DAY_SECONDS * 7 | ||
p = 9 | ||
elif unit == 'D' or unit == 'd': | ||
m = 1000000000L * DAY_SECONDS | ||
p = 9 | ||
elif unit == 'h': | ||
m = 1000000000L * 3600 | ||
p = 9 | ||
elif unit == 'm': | ||
m = 1000000000L * 60 | ||
p = 9 | ||
elif unit == 's': | ||
m = 1000000000L | ||
p = 9 | ||
elif unit == 'ms': | ||
m = 1000000L | ||
p = 6 | ||
elif unit == 'us': | ||
m = 1000L | ||
p = 3 | ||
elif unit == 'ns' or unit is None: | ||
m = 1L | ||
p = 0 | ||
else: | ||
raise ValueError("cannot cast unit {unit}".format(unit=unit)) | ||
|
||
return m, p | ||
|
||
|
||
cdef inline int64_t cast_from_unit(object ts, object unit) except? -1: | ||
""" return a casting of the unit represented to nanoseconds | ||
round the fractional part of a float to our precision, p """ | ||
|
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.
loosing -> losing