-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix Index __mul__-like ops with timedelta scalars #19333
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
Changes from 8 commits
fecc906
7e8d5ff
d2423ca
1dd4ccf
71811c8
4c2c74b
d45ba92
4abd61a
11424b0
ee48c8b
1c97489
23b7f44
086be08
6e04aba
1012939
2fb3688
0ed9276
e798653
a04f970
878be31
d62983d
040b1e4
d7dfba9
a46acb9
e6fa53b
814e858
080c06c
22d155e
9e3dec4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
import numpy as np | ||
from pandas._libs import (lib, index as libindex, tslib as libts, | ||
algos as libalgos, join as libjoin, | ||
Timestamp) | ||
Timestamp, Timedelta) | ||
from pandas._libs.lib import is_datetime_array | ||
|
||
from pandas.compat import range, u, set_function_name | ||
|
@@ -16,7 +16,7 @@ | |
from pandas.core.dtypes.generic import ( | ||
ABCSeries, ABCDataFrame, | ||
ABCMultiIndex, | ||
ABCPeriodIndex, | ||
ABCPeriodIndex, ABCTimedeltaIndex, | ||
ABCDateOffset) | ||
from pandas.core.dtypes.missing import isna, array_equivalent | ||
from pandas.core.dtypes.common import ( | ||
|
@@ -3864,7 +3864,21 @@ def dropna(self, how='any'): | |
return self._shallow_copy() | ||
|
||
def _evaluate_with_timedelta_like(self, other, op, opstr, reversed=False): | ||
raise TypeError("can only perform ops with timedelta like values") | ||
# Timedelta knows how to operate with np.array, so dispatch to that | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be left alone, and rather define this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This for for numeric-dtyped indexes. e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why doesn't this just raise NotImplementedError? which then Timedelta would handle? or is that too recursive a path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For one thing b/c there’s pytimedelta and timedelta64 that need to be handled. |
||
# operation and then wrap the results | ||
other = Timedelta(other) | ||
values = self.values | ||
if reversed: | ||
values, other = other, values | ||
|
||
with np.errstate(all='ignore'): | ||
result = op(values, other) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add some comments (or expand the top ones), this code does a lot |
||
|
||
attrs = self._get_attributes_dict() | ||
attrs = self._maybe_update_attributes(attrs) | ||
if op == divmod: | ||
return Index(result[0], **attrs), Index(result[1], **attrs) | ||
return Index(result, **attrs) | ||
|
||
def _evaluate_with_datetime_like(self, other, op, opstr): | ||
raise TypeError("can only perform ops with datetime like values") | ||
|
@@ -4024,6 +4038,9 @@ def _make_evaluate_binop(op, opstr, reversed=False, constructor=Index): | |
def _evaluate_numeric_binop(self, other): | ||
if isinstance(other, (ABCSeries, ABCDataFrame)): | ||
return NotImplemented | ||
elif isinstance(other, ABCTimedeltaIndex): | ||
# Defer to subclass implementation | ||
return NotImplemented | ||
|
||
other = self._validate_for_numeric_binop(other, op, opstr) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,7 +13,7 @@ | |
import pandas.util.testing as tm | ||
|
||
import pandas as pd | ||
from pandas._libs.lib import Timestamp | ||
from pandas._libs.lib import Timestamp, Timedelta | ||
|
||
from pandas.tests.indexes.common import Base | ||
|
||
|
@@ -26,6 +26,36 @@ def full_like(array, value): | |
return ret | ||
|
||
|
||
class TestIndexArithmetic(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. rename this class its index with timedelta scalar There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment. future readers are going to have no idea of where to find tests |
||
@pytest.mark.parametrize('index', [pd.Int64Index(range(1, 11)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add the issue number as a comment (or PR number if no issue) |
||
pd.UInt64Index(range(1, 11)), | ||
pd.Float64Index(range(1, 11)), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this how we are importing in this module, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like I'm being inconsistent here, will change. |
||
pd.RangeIndex(1, 11)]) | ||
@pytest.mark.parametrize('scalar_td', [Timedelta(days=1), | ||
Timedelta(days=1).to_timedelta64(), | ||
Timedelta(days=1).to_pytimedelta()]) | ||
def test_index_mul_timedelta(self, scalar_td, index): | ||
expected = pd.timedelta_range('1 days', '10 days') | ||
|
||
result = index * scalar_td | ||
tm.assert_index_equal(result, expected) | ||
commute = scalar_td * index | ||
tm.assert_index_equal(commute, expected) | ||
|
||
@pytest.mark.parametrize('index', [pd.Int64Index(range(1, 3)), | ||
pd.UInt64Index(range(1, 3)), | ||
pd.Float64Index(range(1, 3)), | ||
pd.RangeIndex(1, 3)]) | ||
@pytest.mark.parametrize('scalar_td', [Timedelta(days=1), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. these tests really belong with the scalar timedelta tests I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yah I thought about that and eventually decided that since the affected code was in the Index class, the test belonged in the Index tests. But either way works for me. |
||
Timedelta(days=1).to_timedelta64(), | ||
Timedelta(days=1).to_pytimedelta()]) | ||
def test_index_rdiv_timedelta(self, scalar_td, index): | ||
expected = pd.TimedeltaIndex(['1 Day', '12 Hours']) | ||
|
||
result = scalar_td / index | ||
tm.assert_index_equal(result, expected) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you also test that the commute raises here |
||
|
||
|
||
class Numeric(Base): | ||
|
||
def test_numeric_compat(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -297,7 +297,7 @@ def test_dti_mul_dti_raises(self): | |
|
||
def test_dti_mul_too_short_raises(self): | ||
idx = self._holder(np.arange(5, dtype='int64')) | ||
with pytest.raises(ValueError): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the problem is now caught in TimedeltaIndex when checking what it is operating against, whereas before the problem was caught in numpy when trying to operate on differently-sized arrays. |
||
with pytest.raises(TypeError): | ||
idx * self._holder(np.arange(3)) | ||
with pytest.raises(ValueError): | ||
idx * np.array([1, 2]) | ||
|
@@ -476,6 +476,20 @@ def test_ops_compat(self): | |
# don't allow division by NaT (make could in the future) | ||
pytest.raises(TypeError, lambda: rng / pd.NaT) | ||
|
||
@pytest.mark.parametrize('other', [np.arange(1, 11), | ||
pd.Int64Index(range(1, 11)), | ||
pd.UInt64Index(range(1, 11)), | ||
pd.Float64Index(range(1, 11)), | ||
pd.RangeIndex(1, 11)]) | ||
def test_tdi_rmul_arraylike(self, other): | ||
tdi = TimedeltaIndex(['1 Day'] * 10) | ||
expected = timedelta_range('1 days', '10 days') | ||
|
||
result = other * tdi | ||
tm.assert_index_equal(result, expected) | ||
commute = tdi * other | ||
tm.assert_index_equal(commute, expected) | ||
|
||
def test_subtraction_ops(self): | ||
# with datetimes/timedelta and tdi/dti | ||
tdi = TimedeltaIndex(['1 days', pd.NaT, '2 days'], name='foo') | ||
|
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.
when you don't have an issue number, use the PR number