-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG/TST: timedelta-like with Index/Series/DataFrame ops #23320
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 all commits
23fcf74
7755b24
b3a3168
92f1eee
8217977
2a11d9b
7ab4bde
8d96c70
a0be114
481bb90
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 |
---|---|---|
|
@@ -130,6 +130,13 @@ def maybe_upcast_for_op(obj): | |
# implementation; otherwise operation against numeric-dtype | ||
# raises TypeError | ||
return pd.Timedelta(obj) | ||
elif isinstance(obj, np.timedelta64) and not isna(obj): | ||
# In particular non-nanosecond timedelta64 needs to be cast to | ||
# nanoseconds, or else we get undesired behavior like | ||
# np.timedelta64(3, 'D') / 2 == np.timedelta64(1, 'D') | ||
# The isna check is to avoid casting timedelta64("NaT"), which would | ||
# return NaT and incorrectly be treated as a datetime-NaT. | ||
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. Regarding this 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. Yes. In the past when I've brought up the idea of having something like 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. Was testing a few things on master, this seems a bug:
But if I remove here 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. Good catch. I'll get a PR to fix+test this going sometime today. |
||
return pd.Timedelta(obj) | ||
elif isinstance(obj, np.ndarray) and is_timedelta64_dtype(obj): | ||
# GH#22390 Unfortunately we need to special-case right-hand | ||
# timedelta64 dtypes because numpy casts integer dtypes to | ||
|
@@ -1405,11 +1412,12 @@ def wrapper(left, right): | |
index=left.index, name=res_name, | ||
dtype=result.dtype) | ||
|
||
elif is_timedelta64_dtype(right) and not is_scalar(right): | ||
# i.e. exclude np.timedelta64 object | ||
elif is_timedelta64_dtype(right): | ||
# We should only get here with non-scalar or timedelta64('NaT') | ||
# values for right | ||
# Note: we cannot use dispatch_to_index_op because | ||
# that may incorrectly raise TypeError when we | ||
# should get NullFrequencyError | ||
# that may incorrectly raise TypeError when we | ||
# should get NullFrequencyError | ||
result = op(pd.Index(left), right) | ||
return construct_result(left, result, | ||
index=left.index, name=res_name, | ||
|
@@ -1941,8 +1949,7 @@ def f(self, other): | |
|
||
# straight boolean comparisons we want to allow all columns | ||
# (regardless of dtype to pass thru) See #4537 for discussion. | ||
res = self._combine_const(other, func, | ||
errors='ignore') | ||
res = self._combine_const(other, func) | ||
return res.fillna(True).astype(bool) | ||
|
||
f.__name__ = op_name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -148,15 +148,11 @@ def test_numeric_arr_mul_tdscalar(self, scalar_td, numeric_idx, box): | |
tm.assert_equal(commute, expected) | ||
|
||
def test_numeric_arr_rdiv_tdscalar(self, three_days, numeric_idx, box): | ||
index = numeric_idx[1:3] | ||
|
||
broken = (isinstance(three_days, np.timedelta64) and | ||
three_days.dtype != 'm8[ns]') | ||
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 actually had this in a test ...hahha 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, this is to xfail the broken stuff this PR fixes |
||
broken = broken or isinstance(three_days, pd.offsets.Tick) | ||
if box is not pd.Index and broken: | ||
# np.timedelta64(3, 'D') / 2 == np.timedelta64(1, 'D') | ||
raise pytest.xfail("timedelta64 not converted to nanos; " | ||
"Tick division not implemented") | ||
if box is not pd.Index and isinstance(three_days, pd.offsets.Tick): | ||
raise pytest.xfail("Tick division not implemented") | ||
|
||
index = numeric_idx[1:3] | ||
|
||
expected = TimedeltaIndex(['3 Days', '36 Hours']) | ||
|
||
|
@@ -169,6 +165,26 @@ def test_numeric_arr_rdiv_tdscalar(self, three_days, numeric_idx, box): | |
with pytest.raises(TypeError): | ||
index / three_days | ||
|
||
@pytest.mark.parametrize('other', [ | ||
pd.Timedelta(hours=31), | ||
pd.Timedelta(hours=31).to_pytimedelta(), | ||
pd.Timedelta(hours=31).to_timedelta64(), | ||
pd.Timedelta(hours=31).to_timedelta64().astype('m8[h]'), | ||
np.timedelta64('NaT'), | ||
np.timedelta64('NaT', 'D'), | ||
pd.offsets.Minute(3), | ||
pd.offsets.Second(0)]) | ||
def test_add_sub_timedeltalike_invalid(self, numeric_idx, other, box): | ||
left = tm.box_expected(numeric_idx, box) | ||
with pytest.raises(TypeError): | ||
left + other | ||
with pytest.raises(TypeError): | ||
other + left | ||
with pytest.raises(TypeError): | ||
left - other | ||
with pytest.raises(TypeError): | ||
other - left | ||
|
||
|
||
# ------------------------------------------------------------------ | ||
# Arithmetic | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1243,7 +1243,6 @@ def test_binop_other(self, op, value, dtype): | |
(operator.mul, '<M8[ns]'), | ||
(operator.add, '<M8[ns]'), | ||
(operator.pow, '<m8[ns]'), | ||
(operator.mod, '<m8[ns]'), | ||
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 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 testing basically-wrong behavior. |
||
(operator.mul, '<m8[ns]')} | ||
|
||
if (op, dtype) in invalid: | ||
|
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.
Actually, I think we should use here the name of the dtype instead of the class, as this will bubble up for Series as well
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 update in follow-up PR that addresses the bug above.