-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
PERF: array_strptime #55898
PERF: array_strptime #55898
Conversation
@@ -723,6 +775,148 @@ cdef tzinfo _parse_with_format( | |||
return tz | |||
|
|||
|
|||
def _array_strptime_object_fallback( | |||
ndarray[object] values, |
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.
Might need to be careful with performance of this during transition from Cython 0.29 to Cython 3. Definitely if this was just ndarray values
you would lose performance if you didn't use PySequence_GetItem
in the loop; I'm not sure if declaring it as ndarray[object] values
makes a difference
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.
should i update this to use PySequence_GetItem? im not too concerned with this function since it will be removed in 3.0 anyway
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.
If it's going away soon probably not worth the effort
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.
Changes look good
There's a lot of duplication between _array_strptime_object_fallback
and array_strptime
, but as far as I understand, _array_strptime_object_fallback
can be removed in 3.0
anyway, right? If so, no bother
Does this need a whatsnew note for the perf improvement? (can be done as part of another PR, feel free to merge)
correct. |
Thanks @jbrockmendel |
array_strptime is conceptually similar to array_to_datetime but stores local values instead of UTC values, then we pass the result through a localization pass that goes through object dtype. The upside of doing it the current way is that array_strptime does not need a fallback path for when mixed tzs/awareness is detected. The downside is everything else.
This changes array_strptime to store UTC values and follow the same pattern used by array_to_datetime (hopefully we can move toward sharing more code, though some of that will have to wait until deprecations are enforced in 3.0). It introduces a fallback _array_strptime_object_fallback that mirrors _array_to_datetime_object that we'll be able to get rid of once the mixed-tz deprecation is enforced in 3.0.