-
-
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
Fastpaths for Timestamp properties #18539
Conversation
Or just against master(ish):
|
Codecov Report
@@ Coverage Diff @@
## master #18539 +/- ##
==========================================
- Coverage 91.35% 91.33% -0.02%
==========================================
Files 164 164
Lines 49801 49802 +1
==========================================
- Hits 45494 45487 -7
- Misses 4307 4315 +8
Continue to review full report at Codecov.
|
can you confirm we have tests for all of these for both with and w/o freq and asv's for same
|
@@ -304,10 +304,12 @@ cdef class _Timestamp(datetime): | |||
out = get_date_field(np.array([val], dtype=np.int64), field) | |||
return int(out[0]) | |||
|
|||
cpdef _get_start_end_field(self, field): | |||
cpdef bint _get_start_end_field(self, str field): |
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 why is this in _Timestamp again (as opposed to Timestamp); this is why its cpdef, why not just cdef?
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.
Not sure why its in _Timestamp instead of Timestamp (though my understanding is putting it in _Timestamp is slightly more performant and smaller memory footprint); it was that way before I got here.
It is cpdef
and not cdef
because if it were cdef then calling it from Timestamp would be an AttributeError. That's why #18446 moved a bunch of properties up to _Timestamp after making it cdef.
Just pushed with these added. |
thanks! |
Addresses a bunch of the TimestampProperties regressions in #18532 . ASV vs 0.21.0
Timestamps with freqs are still SOL, but that's a relatively rare case.