Use ._tshift internally for datetimelike ops #22949
Conversation
In preperation for PeriodArray / DatetimeArray / TimedeltaArray. Index.shift has a different meaning from ExtensionArray.shift. - Index.shift pointwise shifts each element by some amount - ExtensionArray.shift shits the *position* of each value in the array padding the end with NA This is going to get confusing. This PR tries to avoid some of that by internally using a new `_tshift` method (time-shift) when we want to do pointwise shifting of each value. Places that know they want that behavior (like in the datetimelike ops) should use that.
TomAugspurger
added
Refactor
|
Hello @TomAugspurger! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #22949 +/- ##
==========================================
+ Coverage 92.18% 92.18% +<.01%
==========================================
Files 169 169
Lines 50823 50827 +4
==========================================
+ Hits 46853 46857 +4
Misses 3970 3970
Continue to review full report at Codecov.
|
pandas/core/arrays/datetimelike.py
Outdated
| @@ -553,6 +553,23 @@ def shift(self, periods, freq=None): | |||
| -------- | |||
| Index.shift : Shift values of Index. | |||
| """ | |||
| return self._tshift(periods=periods, freq=freq) | |||
|
|
|||
| def _tshift(self, periods, freq=None): | |||
There was a problem hiding this comment.
why don't we call this ._time_shift or ._period_shift
There was a problem hiding this comment.
if you are actually going to change this, then this should be much more consistent across the code base, e.g. does internals need changing, what about the Index classes?
There was a problem hiding this comment.
I chose _tshift to match Series.tshift, which has the same behavior, but for the Series' index.
There was a problem hiding this comment.
we have a note to deprecate tshift entirely. I don't think this is a good name at all.
| """ | ||
| Shift each value by `periods`. | ||
|
|
||
| Note this is different from ExtensionArray.shift, which |
There was a problem hiding this comment.
how is this relevant here? this is simply a re-definition of what shift means. I understand the desire to make things clear, but this has always been the case, for example in comparing a DTI to an Index.
pandas/core/arrays/datetimelike.py
Outdated
| @@ -553,6 +553,23 @@ def shift(self, periods, freq=None): | |||
| -------- | |||
| Index.shift : Shift values of Index. | |||
| """ | |||
| return self._tshift(periods=periods, freq=freq) | |||
|
|
|||
| def _tshift(self, periods, freq=None): | |||
There was a problem hiding this comment.
if you are actually going to change this, then this should be much more consistent across the code base, e.g. does internals need changing, what about the Index classes?
|
These changes cover the places known to hit PeriodIndex.shift, the places I needed to change on the PeriodArray refactor to get tests passing. I don't see any uses in Internals, and no other Index classes I'm aware of actually implement |
|
Renamed to
Do you have particular places in mind? |
|
All green. |
|
thanks |
In preperation for PeriodArray / DatetimeArray / TimedeltaArray.
Index.shift has a different meaning from ExtensionArray.shift.
padding the end with NA
This is going to get confusing. This PR tries to avoid some of that by
internally using a new
_tshiftmethod (time-shift) when we want to do pointwiseshifting of each value. Places that know they want that behavior (like in the
datetimelike ops) should use that.