-
-
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 14 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 |
---|---|---|
|
@@ -32,6 +32,7 @@ | |
from pandas.core import common as com, algorithms | ||
from pandas.core.algorithms import checked_add_with_arr | ||
from pandas.core.common import AbstractMethodError | ||
from pandas.errors import NullFrequencyError | ||
|
||
import pandas.io.formats.printing as printing | ||
from pandas._libs import lib, iNaT, NaT | ||
|
@@ -692,6 +693,9 @@ def __add__(self, other): | |
return self._add_datelike(other) | ||
elif isinstance(other, Index): | ||
return self._add_datelike(other) | ||
elif is_integer_dtype(other) and self.freq is None: | ||
# GH#19123 | ||
raise NullFrequencyError("Cannot shift with no freq") | ||
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 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 commentThe 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 commentThe 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 |
||
else: # pragma: no cover | ||
return NotImplemented | ||
|
||
|
@@ -731,7 +735,9 @@ def __sub__(self, other): | |
raise TypeError("cannot subtract {typ1} and {typ2}" | ||
.format(typ1=type(self).__name__, | ||
typ2=type(other).__name__)) | ||
|
||
elif is_integer_dtype(other) and self.freq is None: | ||
# GH#19123 | ||
raise NullFrequencyError("Cannot shift with no freq") | ||
else: # pragma: no cover | ||
return NotImplemented | ||
|
||
|
@@ -831,7 +837,7 @@ def shift(self, n, freq=None): | |
return self | ||
|
||
if self.freq is None: | ||
raise ValueError("Cannot shift with no freq") | ||
raise NullFrequencyError("Cannot shift with no freq") | ||
|
||
start = self[0] + n * self.freq | ||
end = self[-1] + n * self.freq | ||
|
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,40 @@ 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. | ||
if left_idx.freq is not None: | ||
left_idx = left_idx._shallow_copy(freq=None) | ||
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 |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
|
||
from itertools import product | ||
import pandas as pd | ||
from pandas.errors import NullFrequencyError | ||
import pandas._libs.tslib as tslib | ||
from pandas._libs.tslibs.offsets import shift_months | ||
import pandas.util.testing as tm | ||
|
@@ -593,6 +594,12 @@ def test_nat_new(self): | |
exp = np.array([tslib.iNaT] * 5, dtype=np.int64) | ||
tm.assert_numpy_array_equal(result, exp) | ||
|
||
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. there are very likely tests that are raising ValueError that should now be NullFrequencyError, pls change them. You can find them very easily, by temporarly changing the error where NullFrequencyInhertis to something else (e.g. KeyError) and then seeing what raises 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. Will do. 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. Found and changed one. 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 great |
||
def test_shift_no_freq(self): | ||
# GH#19147 | ||
dti = pd.DatetimeIndex(['2011-01-01 10:00', '2011-01-01'], freq=None) | ||
with pytest.raises(NullFrequencyError): | ||
dti.shift(2) | ||
|
||
def test_shift(self): | ||
# GH 9903 | ||
for tz in self.tz: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -693,6 +693,25 @@ def test_td64series_rsub_int_series_invalid(self, tdser): | |
with pytest.raises(TypeError): | ||
Series([2, 3, 4]) - tdser | ||
|
||
def test_td64_series_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)) | ||
|
||
@pytest.mark.parametrize('scalar', [1, 1.5, np.array(2)]) | ||
def test_td64series_add_sub_numeric_scalar_invalid(self, scalar, tdser): | ||
with pytest.raises(TypeError): | ||
|
@@ -1533,6 +1552,26 @@ def test_dt64_series_arith_overflow(self): | |
res = dt - ser | ||
tm.assert_series_equal(res, -expected) | ||
|
||
@pytest.mark.parametrize('tz', [None, 'Asia/Tokyo']) | ||
def test_dt64_series_add_intlike(self, tz): | ||
# GH#19123 | ||
dti = pd.DatetimeIndex(['2016-01-02', '2016-02-03', 'NaT'], tz=tz) | ||
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): | ||
|
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.
say something like: which subclasses ValueError, the currently raised exception
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.
Will do.