-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
Fix incorrect exception raised by Series[datetime64] + int #19147
Changes from 5 commits
1241994
1fe5732
0aee07a
29a4931
4fd68c1
efcde8e
df545a0
1a03a68
878e689
fd0ac99
a623d00
aab851d
66d6bce
15b5f08
63ae039
edebbe2
2231505
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 |
---|---|---|
|
@@ -20,7 +20,7 @@ | |
from pandas.compat import bind_method | ||
import pandas.core.missing as missing | ||
|
||
from pandas.errors import PerformanceWarning | ||
from pandas.errors import PerformanceWarning, NullFrequencyError | ||
from pandas.core.common import _values_from_object, _maybe_match_name | ||
from pandas.core.dtypes.missing import notna, isna | ||
from pandas.core.dtypes.common import ( | ||
|
@@ -672,9 +672,8 @@ def wrapper(left, right, name=name, na_op=na_op): | |
|
||
left, right = _align_method_SERIES(left, right) | ||
if is_datetime64_dtype(left) or is_datetime64tz_dtype(left): | ||
result = op(pd.DatetimeIndex(left), right) | ||
result = dispatch_to_index_op(op, left, right, pd.DatetimeIndex) | ||
res_name = _get_series_op_result_name(left, right) | ||
result.name = res_name # needs to be overriden if None | ||
return construct_result(left, result, | ||
index=left.index, name=res_name, | ||
dtype=result.dtype) | ||
|
@@ -703,6 +702,39 @@ def wrapper(left, right, name=name, na_op=na_op): | |
return wrapper | ||
|
||
|
||
def dispatch_to_index_op(op, left, right, index_class): | ||
""" | ||
Wrap Series left in the given index_class to delegate the operation op | ||
to the index implementation. DatetimeIndex and TimedeltaIndex perform | ||
type checking, timezone handling, overflow checks, etc. | ||
|
||
Parameters | ||
---------- | ||
op : binary operator (operator.add, operator.sub, ...) | ||
left : Series | ||
right : object | ||
index_class : DatetimeIndex or TimedeltaIndex | ||
|
||
Returns | ||
------- | ||
result : object, usually DatetimeIndex, TimedeltaIndex, or Series | ||
""" | ||
left_idx = index_class(left) | ||
|
||
# avoid accidentally allowing integer add/sub. For datetime64[tz] dtypes, | ||
# left_idx may inherit a freq from a cached DatetimeIndex. | ||
# See discussion in GH#19147. | ||
left_idx.freq = None | ||
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. actually I don't like this mutation here. why is it necessary? (e.g. if one of the end-points is None, e.g. an integer series this will still raise, yes?) 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 if
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. and why is this a problem? 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 in this scenario, without the 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. Just parametrized 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. so you didn't answer my question here. I don't want mutation. 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. I really, really did. Without setting the frequency to none, integer addition fails to raise. 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. then we need a better way to detect this, we cannot mutate things to just see if they raise |
||
try: | ||
result = op(left_idx, right) | ||
except NullFrequencyError: | ||
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. ok this is fine then. Please update tests where this should be caught (e.g. .shift) |
||
# DatetimeIndex and TimedeltaIndex with freq == None raise ValueError | ||
# on add/sub of integers (or int-like). We re-raise as a TypeError. | ||
raise TypeError('incompatible type for a datetime/timedelta ' | ||
'operation [{name}]'.format(name=op.__name__)) | ||
return result | ||
|
||
|
||
def _get_series_op_result_name(left, right): | ||
# `left` is always a pd.Series | ||
if isinstance(right, (ABCSeries, pd.Index)): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,3 +65,11 @@ class MergeError(ValueError): | |
Error raised when problems arise during merging due to problems | ||
with input data. Subclass of `ValueError`. | ||
""" | ||
|
||
|
||
class NullFrequencyError(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. this needs a line in the api changes section 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. done. |
||
Error raised when a null `freq` attribute is used in an operation | ||
that needs a non-null frequency, particularly `DatetimeIndex.shift`, | ||
`TimedeltaIndex.shift`, `PeriodIndex.shift`. | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -680,6 +680,25 @@ def test_timedelta_series_ops(self): | |
assert_series_equal(ts - s, expected2) | ||
assert_series_equal(ts + (-s), expected2) | ||
|
||
def test_td64series_add_intlike(self): | ||
# GH#19123 | ||
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. don't conjoin words test_td64_series_and_integer_like |
||
tdi = pd.TimedeltaIndex(['59 days', '59 days', 'NaT']) | ||
ser = Series(tdi) | ||
|
||
other = Series([20, 30, 40], dtype='uint8') | ||
|
||
pytest.raises(TypeError, ser.__add__, 1) | ||
pytest.raises(TypeError, ser.__sub__, 1) | ||
|
||
pytest.raises(TypeError, ser.__add__, other) | ||
pytest.raises(TypeError, ser.__sub__, other) | ||
|
||
pytest.raises(TypeError, ser.__add__, other.values) | ||
pytest.raises(TypeError, ser.__sub__, other.values) | ||
|
||
pytest.raises(TypeError, ser.__add__, pd.Index(other)) | ||
pytest.raises(TypeError, ser.__sub__, pd.Index(other)) | ||
|
||
def test_timedelta64_operations_with_integers(self): | ||
# GH 4521 | ||
# divide/multiply by integers | ||
|
@@ -739,12 +758,6 @@ def test_timedelta64_operations_with_integers(self): | |
Series([Timedelta('29 days 12:00:00'), Timedelta( | ||
'29 days 12:00:00'), Timedelta('NaT')])) | ||
|
||
for op in ['__add__', '__sub__']: | ||
sop = getattr(s1, op, None) | ||
if sop is not None: | ||
pytest.raises(TypeError, sop, 1) | ||
pytest.raises(TypeError, sop, s2.values) | ||
|
||
def test_timedelta64_operations_with_DateOffset(self): | ||
# GH 10699 | ||
td = Series([timedelta(minutes=5, seconds=3)] * 3) | ||
|
@@ -1428,6 +1441,25 @@ def test_dt64series_arith_overflow(self): | |
res = dt - ser | ||
tm.assert_series_equal(res, -expected) | ||
|
||
def test_dt64series_add_intlike(self): | ||
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. same |
||
# GH#19123 | ||
dti = pd.DatetimeIndex(['2016-01-02', '2016-02-03', 'NaT']) | ||
ser = Series(dti) | ||
|
||
other = Series([20, 30, 40], dtype='uint8') | ||
|
||
pytest.raises(TypeError, ser.__add__, 1) | ||
pytest.raises(TypeError, ser.__sub__, 1) | ||
|
||
pytest.raises(TypeError, ser.__add__, other) | ||
pytest.raises(TypeError, ser.__sub__, other) | ||
|
||
pytest.raises(TypeError, ser.__add__, other.values) | ||
pytest.raises(TypeError, ser.__sub__, other.values) | ||
|
||
pytest.raises(TypeError, ser.__add__, pd.Index(other)) | ||
pytest.raises(TypeError, ser.__sub__, pd.Index(other)) | ||
|
||
|
||
class TestSeriesOperators(TestData): | ||
def test_op_method(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1233,7 +1233,8 @@ def _get_roll(self, i, before_day_of_month, after_day_of_month): | |
return roll | ||
|
||
def _apply_index_days(self, i, roll): | ||
i += (roll % 2) * Timedelta(days=self.day_of_month).value | ||
nanos = (roll % 2) * Timedelta(days=self.day_of_month).value | ||
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. leftover from another PR (and I had commented on that), remove from here. 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. You’re rIght that this is duplicated, but it is correct in both places, and this PR breaks without it. 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. see other PR, this needs a doc-string |
||
i += nanos.astype('timedelta64[ns]') | ||
return i + Timedelta(days=-1) | ||
|
||
|
||
|
@@ -1278,7 +1279,8 @@ def _get_roll(self, i, before_day_of_month, after_day_of_month): | |
return roll | ||
|
||
def _apply_index_days(self, i, roll): | ||
return i + (roll % 2) * Timedelta(days=self.day_of_month - 1).value | ||
nanos = (roll % 2) * Timedelta(days=self.day_of_month - 1).value | ||
return i + nanos.astype('timedelta64[ns]') | ||
|
||
|
||
# --------------------------------------------------------------------- | ||
|
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.
just make this a ValueError, we don't want to have custom error messages normally.
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 need to catch this specifically in core.ops. Want to catch by checking the error message?
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.
The error message here "Cannot shift with no freq" is the same as the error message raised if adding just an integer when
self.freq
is None.