-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
Fix Series[timedelta64]+DatetimeIndex[tz] bugs #18884
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 19 commits
8e856d1
2e824bc
5471d40
d5a0862
e142946
277b8cb
5a5d98c
76582ac
5c2e883
ae3b34d
e72bcf9
68727d4
dd2970c
26c9b51
a7a3d6f
3e71017
dead356
51234b3
7b831ff
c09ac0f
0fc30bb
ae1af46
190197c
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 |
---|---|---|
|
@@ -39,7 +39,7 @@ | |
from pandas.core.dtypes.generic import ( | ||
ABCSeries, | ||
ABCDataFrame, | ||
ABCIndex, | ||
ABCIndex, ABCDatetimeIndex, | ||
ABCPeriodIndex) | ||
|
||
# ----------------------------------------------------------------------------- | ||
|
@@ -514,8 +514,9 @@ def _convert_to_array(self, values, name=None, other=None): | |
values[:] = iNaT | ||
|
||
# a datelike | ||
elif isinstance(values, pd.DatetimeIndex): | ||
values = values.to_series() | ||
elif isinstance(values, ABCDatetimeIndex): | ||
# TODO: why are we casting to_series in the first place? | ||
values = values.to_series(keep_tz=True) | ||
# datetime with tz | ||
elif (isinstance(ovalues, datetime.datetime) and | ||
hasattr(ovalues, 'tzinfo')): | ||
|
@@ -535,6 +536,11 @@ def _convert_to_array(self, values, name=None, other=None): | |
elif inferred_type in ('timedelta', 'timedelta64'): | ||
# have a timedelta, convert to to ns here | ||
values = to_timedelta(values, errors='coerce', box=False) | ||
if isinstance(other, ABCDatetimeIndex): | ||
# GH#13905 | ||
# Defer to DatetimeIndex/TimedeltaIndex operations where | ||
# timezones are handled carefully. | ||
values = pd.TimedeltaIndex(values) | ||
elif inferred_type == 'integer': | ||
# py3 compat where dtype is 'm' but is an integer | ||
if values.dtype.kind == 'm': | ||
|
@@ -752,31 +758,36 @@ def wrapper(left, right, name=name, na_op=na_op): | |
na_op = converted.na_op | ||
|
||
if isinstance(rvalues, ABCSeries): | ||
name = _maybe_match_name(left, rvalues) | ||
lvalues = getattr(lvalues, 'values', lvalues) | ||
rvalues = getattr(rvalues, 'values', rvalues) | ||
# _Op aligns left and right | ||
else: | ||
if isinstance(rvalues, pd.Index): | ||
name = _maybe_match_name(left, rvalues) | ||
else: | ||
name = left.name | ||
if (hasattr(lvalues, 'values') and | ||
not isinstance(lvalues, pd.DatetimeIndex)): | ||
not isinstance(lvalues, ABCDatetimeIndex)): | ||
lvalues = lvalues.values | ||
|
||
res_name = _get_series_op_result_name(left, right) | ||
result = wrap_results(safe_na_op(lvalues, rvalues)) | ||
return construct_result( | ||
left, | ||
result, | ||
index=left.index, | ||
name=name, | ||
name=res_name, | ||
dtype=dtype, | ||
) | ||
|
||
return wrapper | ||
|
||
|
||
def _get_series_op_result_name(left, right): | ||
# `left` is always a Series object | ||
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 suggested a simplification 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. ok, rather than creating another function like this, just in-line it as its only used once. . |
||
if isinstance(right, (ABCSeries, pd.Index)): | ||
name = _maybe_match_name(left, right) | ||
else: | ||
name = left.name | ||
return name | ||
|
||
|
||
def _comp_method_OBJECT_ARRAY(op, x, y): | ||
if isinstance(y, list): | ||
y = construct_1d_object_array_from_listlike(y) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -363,6 +363,27 @@ def test_datetimeindex_sub_timestamp_overflow(self): | |
with pytest.raises(OverflowError): | ||
dtimin - variant | ||
|
||
@pytest.mark.parametrize('names', [('foo', None, None), | ||
('baz', 'bar', None), | ||
('bar', 'bar', 'bar')]) | ||
@pytest.mark.parametrize('tz', [None, 'America/Chicago']) | ||
def test_dti_add_series(self, tz, names): | ||
# GH#13905 | ||
index = DatetimeIndex(['2016-06-28 05:30', '2016-06-28 05:31'], | ||
tz=tz, name=names[0]) | ||
ser = Series([Timedelta(seconds=5)] * 2, | ||
index=index, name=names[1]) | ||
expected = Series(index + Timedelta(seconds=5), | ||
index=index, name=names[2]) | ||
|
||
# passing name arg isn't enough when names[2] is 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. pls add blank lines before comments |
||
expected.name = names[2] | ||
assert expected.dtype == index.dtype | ||
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. can you also test adding with |
||
result = ser + index | ||
tm.assert_series_equal(result, expected) | ||
result2 = index + ser | ||
tm.assert_series_equal(result2, expected) | ||
|
||
@pytest.mark.parametrize('box', [np.array, pd.Index]) | ||
def test_dti_add_offset_array(self, tz, box): | ||
# GH#18849 | ||
|
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.
and if you change this does it work?
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.
Tentative yes, but only with the change made by
_get_series_result_name
below.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.
Update: tinkering with this and doing the threading requested below don't play nicely together. We should keep this as-is and address separately. This fixes a bug and we should call it a win. There are a bunch of these bug-fixes to get in before we can clean up the mess that is _TimeOp.