-
-
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 10 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 |
---|---|---|
|
@@ -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 |
---|---|---|
|
@@ -697,6 +697,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): | ||
|
@@ -1528,6 +1547,26 @@ def test_dt64series_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.
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.