Pin correct names to wrapped Index comparison methods #18397
Conversation
pandas/core/indexes/base.py
Outdated
| @@ -3890,6 +3890,7 @@ def _evaluate_compare(self, other): | |||
| except TypeError: | |||
| return result | |||
|
|
|||
| _evaluate_compare.__name__ = '__{name}__'.format(name=op.__name__) | |||
There was a problem hiding this comment.
we have a function in core/generic for exactly this
(and sets properly in py2/3 and such)
There was a problem hiding this comment.
Closest I can think of is pd.compat.bind_method. Is that what you're referring to? Wrapped method names are set like this in e.g. indexes.datetimes._field_accessor.
There was a problem hiding this comment.
use pandas.compat.set_function_name
There was a problem hiding this comment.
That's going to get into a circularity problem. set_function_name requires cls, but in several cases this is being called before the class has been defined in the namespace.
Scroll up to line 50 in timedeltas for an example of the status quo usage.
There was a problem hiding this comment.
see how its done in generic.py, instead you should change these to a factor function.
There was a problem hiding this comment.
OK I'll do it, but under protest... those classmethods to _add_foo_methods are among my least favorite design patterns.
There was a problem hiding this comment.
these are already a well established pattern
and they handle the edge cases
eg qualname
There was a problem hiding this comment.
In indexes.base.Index the func.__name__ = name pattern is used in _add_logical_methods, _add_logical_methods_disabled, _add_numeric_methods_disabled, _add_numeric_methods_add_sub_disabled. In case you want to open an issue to change those.
pandas/tests/indexes/test_base.py
Outdated
| index = indices | ||
| assert index.__eq__.__name__ == '__eq__' | ||
| assert index.__ne__.__name__ == '__ne__' | ||
| assert index.__le__.__name__ == '__le__' |
Codecov Report
@@ Coverage Diff @@
## master #18397 +/- ##
==========================================
- Coverage 91.36% 91.34% -0.03%
==========================================
Files 164 164
Lines 49721 49736 +15
==========================================
+ Hits 45429 45432 +3
- Misses 4292 4304 +12
Continue to review full report at Codecov.
|
|
I'm actually coming around to being more OK with the |
|
for everything u can out in test_api |
|
OK, I'll do that separately since a bunch of those fail at the moment, will need to be patched in a bunch of places. This should be good-to-go on its own. |
| __gt__ = _td_index_cmp('__gt__') | ||
| __le__ = _td_index_cmp('__le__') | ||
| __ge__ = _td_index_cmp('__ge__') | ||
| @classmethod |
There was a problem hiding this comment.
shouldn't we define the _add_comparison_method to take a function and live in indexes/base? to avoid this repeated code?
There was a problem hiding this comment.
I'm open to this. It isn't entirely trivial because it references DatetimeIndex, TimedeltaIndex, super... I'll take a look at how much datetimelike-specific logic is there; it may be more at home in indexes.datetimelike.
For now I'd advocate closing this fairly straightforward fix.
There was a problem hiding this comment.
ok pls add to list (opening an issue is ok too, e.g. if its a bigger thing, not likely to get to anytime soon, etc).
There was a problem hiding this comment.
Yah there's definitely some logic shared between the timedeltas and datetimes version, but its a little bit deceptive in part because _to_m8 is defined differently in each module.
I've got an implementation of roughly this. Long story short: we're going to want to solve #18376 before trying to de-repeat this code. |
Currently:
PR:
Same for subclasses and other comparison methods.
On the side: use property decorator for
DatetimeIndex.freqto avoid leaving_get_freqand_set_freqin the namespace.git diff upstream/master -u -- "*.py" | flake8 --diff