REF: Refactor Datetimelike delegation #24039
Conversation
This is a generalization of PeriodIndex's dispatching to PeriodArray, without any actual changes yet. This is split from pandas-dev#24024, where DatetimeIndex and TimedeltaIndex will implement and inherit from delgates similiar to PeriodDelegateMixin.
|
Hello @TomAugspurger! Thanks for submitting the PR.
|
|
These latest two commits have some actual changes:
|
Codecov Report
@@ Coverage Diff @@
## master #24039 +/- ##
===========================================
+ Coverage 42.46% 92.35% +49.88%
===========================================
Files 161 161
Lines 51557 51562 +5
===========================================
+ Hits 21892 47618 +25726
+ Misses 29665 3944 -25721
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24039 +/- ##
===========================================
- Coverage 92.31% 42.46% -49.86%
===========================================
Files 161 161
Lines 51562 51568 +6
===========================================
- Hits 47599 21896 -25703
- Misses 3963 29672 +25709
Continue to review full report at Codecov.
|
| def _delegate_method(self, name, *args, **kwargs): | ||
| result = operator.methodcaller(name, *args, **kwargs)(self._data) | ||
| if name not in self._raw_methods: | ||
| result = Index(result, name=self.name) |
There was a problem hiding this comment.
this will need checks for 2-tuples in divmod and rdivmod, for scalars for eventual reduction ops
There was a problem hiding this comment.
I think all the delegated methods return a single value.
There was a problem hiding this comment.
If #23885 goes through that will change
There was a problem hiding this comment.
Gotcha. I'd prefer to wait until we can hit that code path before worrying about it.
| @@ -855,3 +857,48 @@ def f(self): | |||
| f.__name__ = fget.__name__ | |||
| f.__doc__ = fget.__doc__ | |||
| return property(f) | |||
|
|
|||
|
|
|||
| class DatetimelikeDelegateMixin(PandasDelegate): | |||
There was a problem hiding this comment.
Is PandasDelegate overkill for this situation? It's not like Series.dt where we construct an object and need to cache it. Couldn't we just use something like:
def pass_through_methods(names):
def decorator(cls):
for name in names:
method = wrap_array_method(...)
setattr(cls, name, method)
return cls
There's definitely overlap with the PandasDelegate machinery, but I think that machinery makes for really dense reading, so should be reserved for when we really need it.
There was a problem hiding this comment.
Perhaps you're thinking of Properties (which subclasses PandasDelegate)? PandasDelegate doesn't have an __init__, it just handles registering and creating the delegated properties.
There was a problem hiding this comment.
But, I don't have a strong preference of one of the other. We use PandasDelegate on master, for period, so this PR is a strict generalization of that.
|
lgtm. merge when ready @TomAugspurger @jbrockmendel |
|
LGTM. I might poke at simplifying down the road, but that shouldn't be a blocker. |
|
ok I merged the other one, so @TomAugspurger if you can rebase |
|
lgtm. ping on green. |
|
thanks @TomAugspurger |
This is a generalization of PeriodIndex's dispatching to
PeriodArray, without any actual changes yet. This is split
from #24024, where DatetimeIndex and TimedeltaIndex will
implement and inherit from delgates similiar to PeriodDelegateMixin.