More speedups for Period comparisons #21606
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21606 +/- ##
==========================================
- Coverage 91.9% 91.89% -0.01%
==========================================
Files 154 154
Lines 49562 49543 -19
==========================================
- Hits 45549 45530 -19
Misses 4013 4013
Continue to review full report at Codecov.
|
jreback
added
Performance
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
add this issue to your existing perf issue (always do that on follow ons)
pandas/_libs/tslibs/offsets.pyx
Outdated
|
|
||
| def __eq__(self, other): | ||
| if is_string_object(other): | ||
| from pandas.tseries.frequencies import to_offset |
There was a problem hiding this comment.
maybe you want to define a function to return this import, we use this in a number of places?
There was a problem hiding this comment.
Wouldn't this just be trading one one-liner for another?
There was a problem hiding this comment.
no then then its a function which we can import at the top (and of course if we eventually are able to actually move to_offset to cython its a big win). but its 'cleaner' I think. (certainly can followup on this in new PR). I just don't really like the imports hidden in the functions (as we must do currently), and rather centrailize in a single place.
pandas/_libs/tslibs/offsets.pyx
Outdated
|
|
||
| @property | ||
| def _params(self): | ||
| from pandas.errors import AbstractMethodError |
There was a problem hiding this comment.
can you import this at the top?
jreback
left a comment
jreback
left a comment
There was a problem hiding this comment.
just a minor comment. ping on green.
pandas/_libs/tslibs/offsets.pyx
Outdated
|
|
||
| def __eq__(self, other): | ||
| if is_string_object(other): | ||
| from pandas.tseries.frequencies import to_offset |
There was a problem hiding this comment.
no then then its a function which we can import at the top (and of course if we eventually are able to actually move to_offset to cython its a big win). but its 'cleaner' I think. (certainly can followup on this in new PR). I just don't really like the imports hidden in the functions (as we must do currently), and rather centrailize in a single place.
pandas/tseries/offsets.py
Outdated
| Since 0 is a bit weird, we suggest avoiding its use. | ||
| """ | ||
| _params = cache_readonly(BaseOffset._params.fget) | ||
|
|
There was a problem hiding this comment.
can you remove this blank line
|
|
||
| @property # NB: non-cython subclasses override with cache_readonly | ||
| def _params(self): | ||
| all_paras = self.__dict__.copy() |
There was a problem hiding this comment.
can you give an expl of what this is doing (and you can move the comment inside the function too)
|
thanks @jbrockmendel ! |
2 participants
Following #21582, the biggest avoidable overheads in Period comparisons are in a)
isinstancecalls that can be short-circuited and b)__ne__call overhead. This PR moves__eq__and__ne__to the cython file, removing the__ne__overhead, and changes anisinstancecheck to a try/except.Using the same profile code from #21582, this brings the
set_indexruntime from 6.603 seconds down to 2.165 seconds.