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
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -313,6 +313,30 @@ class _BaseOffset(object): | |||
| def __setattr__(self, name, value): | |||
| raise AttributeError("DateOffset objects are immutable.") | |||
|
|
|||
| 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?
pandas/_libs/tslibs/offsets.pyx
Outdated
| @@ -313,6 +313,30 @@ class _BaseOffset(object): | |||
| def __setattr__(self, name, value): | |||
| raise AttributeError("DateOffset objects are immutable.") | |||
|
|
|||
| 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
| @@ -182,6 +182,8 @@ def __add__(date): | |||
|
|
|||
| 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
pandas/_libs/tslibs/offsets.pyx
Outdated
|
|
||
| @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 ! |
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.