More tslibs TODOS: small optimizations, trim namespaces #18446
Conversation
pandas/_libs/tslibs/timestamps.pyx
Outdated
| @@ -246,6 +249,7 @@ cdef class _Timestamp(datetime): | |||
| result.nanosecond = self.nanosecond | |||
| return result | |||
|
|
|||
| @cython.final | |||
There was a problem hiding this comment.
can you perf check this (you can just compare time-its for couple of ops in PR and master to see); if it makes a diff then add an asv (and if not let's not do this).
There was a problem hiding this comment.
if it makes a diff then add an asv
I'm shocked to see there isn't already an asv for "Timestamp addition/subtraction". I'll make one, or see if one isn't hidden somewhere other than benchmarks/timestamp.py. If it is somewhere else, will moving them around cause a problem for backward-comparability?
can you perf check this
Not seeing much difference, but %timeit results are jumping around more than I'd like. I've got a couple of questions queued up for the cython mailing list, will add this to them.
There was a problem hiding this comment.
I'm shocked to see there isn't already an asv for "Timestamp addition/subtraction". I'll make one, or see if one isn't hidden somewhere other than benchmarks/timestamp.py. If it is somewhere else, will moving them around cause a problem for backward-comparability?
no there are, its just asv's seem to be unstable for you :> (and this is prob a tiny difference)
There was a problem hiding this comment.
no there are
Unless the instability extends to the contents of the benchmarks/ files, I'm pretty sure asv_bench/benchmarks/timestamp.py doesn't have any addition/subtraction benchmarks.
There was a problem hiding this comment.
Yah, so the question about moving was if its OK to consolidate Timestamp asvs (kinda analogous to what I've been doing for the tests)
There was a problem hiding this comment.
yes sure. we definitly want compreshensive benchmarks for scalar + scalar (and scalar + index, etc). so consolidation is fine.
|
Asked on the cython mailing list: turns out |
|
Looks like because of cython implementation details, making |
don't set defaults, this is a required user-settable parameter. move them back to the class impl. not really sure these belong in the c class def anyhow (no benefit). |
Sure. The idea was to get |
Codecov Report
@@ Coverage Diff @@
## master #18446 +/- ##
==========================================
+ Coverage 91.35% 91.53% +0.17%
==========================================
Files 163 163
Lines 49691 50730 +1039
==========================================
+ Hits 45397 46437 +1040
+ Misses 4294 4293 -1
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18446 +/- ##
==========================================
+ Coverage 91.35% 91.53% +0.17%
==========================================
Files 163 163
Lines 49691 50730 +1039
==========================================
+ Hits 45397 46437 +1040
+ Misses 4294 4293 -1
Continue to review full report at Codecov.
|
| def days_in_month(self): | ||
| return self._get_field('dim') | ||
|
|
||
| cdef bint _get_start_end_field(_Timestamp self, field): |
There was a problem hiding this comment.
why does moving this to _Timestamp help
There was a problem hiding this comment.
why does moving this to _Timestamp help
Among the todos was getting _get_start_end_field and _get_field out of the user-facing namespace. This accomplishes that.
It also adds type declarations, so should be marginally more performant.
There was a problem hiding this comment.
why do we care about _get_start_end_field in the user namespace, its a private method?
type declarations are fine (pls run an asv to confirm)
| def days_in_month(self): | ||
| return self._get_field('dim') | ||
|
|
||
| cdef bint _get_start_end_field(_Timestamp self, field): |
There was a problem hiding this comment.
why do we care about _get_start_end_field in the user namespace, its a private method?
type declarations are fine (pls run an asv to confirm)
Not sure anymore. I probably put it on the todo list in my youth. Will mark it as fine-as-is.
OK. I'll close this for now and circle back; avoid clogging the CI queue. |
A few methods of
TimedeltaandTimestampthat don't need to be user-facing are changed fromcpdeftocdef. To make the wheels turn,Timestampmethods that call those methods are moved fromTimestampto_Timestamp.Stronger typing for
_Timestamp. _get_start_end_fieldand a couple others.@cython.finalsupposedly allows for a small optimization because cython does not need to check at runtime whether a method has been overridden. Applied toTimestamp.__add__,Timestamp.__sub__