-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Fix incorrect exception raised by Series[datetime64] + int #19147
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 1 commit
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,23 @@ def wrapper(left, right, name=name, na_op=na_op): | |
return wrapper | ||
|
||
|
||
def _dispatch_to_index_op(op, left, right, index_class): | ||
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. We're going to end up using this again for TimedeltaIndex a few bugfixes from now. 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 can de-privatize. rename to 'dispatch_to_index_operation'. needs a doc-string. |
||
""" | ||
Defer to DatetimeIndex implementations for type | ||
checking and timezone handling. | ||
""" | ||
left_idx = index_class(left) | ||
left_idx.freq = None # avoid accidentally allowing integer add/sub | ||
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 is a mutating operation, don't do 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. This is necessary. If I change this to an
It looks like this only occurs for tzaware cases. 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.
Possibly related to caching of DatetimeIndexes and the fact that Series[datetime64tz]._data.blocks[0].values is itself a DatetimeIndex? |
||
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,24 @@ 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 |
---|---|---|
|
@@ -1226,7 +1226,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) | ||
|
||
|
||
|
@@ -1271,7 +1272,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.