-
-
Notifications
You must be signed in to change notification settings - Fork 17.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement integer array add/sub for datetimelike indexes #19959
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19959 +/- ##
==========================================
+ Coverage 91.83% 91.84% +<.01%
==========================================
Files 153 153
Lines 49498 49515 +17
==========================================
+ Hits 45458 45475 +17
Misses 4040 4040
Continue to review full report at Codecov.
|
pandas/core/indexes/datetimelike.py
Outdated
td = Timedelta(self.freq) | ||
return op(self, td * other) | ||
|
||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for an else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you address this comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fell through the cracks; just pushed an update.
@@ -810,6 +851,11 @@ def __rsub__(self, other): | |||
# we need to wrap in DatetimeIndex and flip the operation | |||
from pandas import DatetimeIndex | |||
return DatetimeIndex(other) - self | |||
elif (is_datetime64_any_dtype(self) and hasattr(other, 'dtype') and | |||
not is_datetime64_any_dtype(other)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment on why this is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed, but GitHub UI not surfacing.
|
||
@pytest.mark.parametrize('freq', ['H', 'D']) | ||
@pytest.mark.parametrize('box', [np.array, pd.Index]) | ||
def test_dti_add_intarray_tick(self, box, freq): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we must have some tests that do index arithmetic with integers, either move them here if they are not redundant or remove them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are tests for integers, but nothing ATM for integer-arrays.
# --------------------------------------------------------------- | ||
# __add__/__sub__ with integer arrays | ||
|
||
@pytest.mark.parametrize('box', [np.array, pd.Index]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as above
Comments above acknowledged. Let's stick a pin in this for a little bit. I think there may be a problem with integer ops, need to double-check before moving forward on this. |
I've convinced myself this is OK. Or more accurately, that weird behavior (not affected by this PR, but in integer add/sub) can only come up if a user specifically passes |
Needs rebasing since its gotten a bit stale, but I think this is otherwise ready. Will push shortly. |
@jreback gentle ping |
rebase as well |
@jreback gentle ping. I've got some time this weekend I can put towards finishing this off. |
@gfyoung any idea if everything/everyone is OK? |
@jbrockmendel : I see a bunch of comments from @jreback that you haven't explicitly addressed. That might be why no one is saying anything. |
@gfyoung Thanks. I think there's just the one where I haven't un-indented an |
EDIT: I see the comment now! 😄 |
@jreback : ping. I think all comments have been addressed. |
@jreback gentle ping. I'd like to finish this off and get back to unifying Index/Series/DataFrame arithmetic implementations in |
will have to wait until after the rc |
@jreback gentle ping |
doc/source/whatsnew/v0.23.0.txt
Outdated
@@ -951,7 +951,7 @@ Datetimelike API Changes | |||
rather than ``NotImplementedError`` when index is not a :class:`DatetimeIndex` (:issue:`20725`). | |||
- Restricted ``DateOffset`` keyword arguments. Previously, ``DateOffset`` subclasses allowed arbitrary keyword arguments which could lead to unexpected behavior. Now, only valid arguments will be accepted. (:issue:`17176`, :issue:`18226`). | |||
- :func:`pandas.merge` provides a more informative error message when trying to merge on timezone-aware and timezone-naive columns (:issue:`15800`) | |||
- For :class:`DatetimeIndex` and :class:`TimedeltaIndex` with ``freq=None``, addition or subtraction of integer-dtyped array or ``Index`` will raise ``NullFrequencyError`` instead of ``TypeError`` (:issue:`19895`) | |||
- For :class:`DatetimeIndex` and :class:`TimedeltaIndex` with ``freq=None``, addition or subtraction of integer-dtyped array or ``Index`` will raise ``NullFrequencyError`` instead of ``TypeError`` if the index ``freq`` attribute is ``None``, and will return an object of the same class otherwise (:issue:`19895`, :issue:`19959`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.24 (this is an api change right?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, just changed.
thanks @jbrockmendel it seems there might be some code you can remove from PeriodIndex though? (obviously in a followup) |
Sounds plausible; I'll have to refresh my memory. IIRC the next dtype to handle is Period/PeriodIndex subtraction, with the complication that we need to fix the current scalar subtraction behavior. |
AFAICT this is the next-to-last non-raising case for these operations. After this is just object-dtype, and for that we already have the correct implementation in addsub_offset_array, just need to make the condition less strict and add appropriate tests.
git diff upstream/master -u -- "*.py" | flake8 --diff