Skip to content

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

Merged
merged 23 commits into from
Jan 2, 2018
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
8e856d1
Fix Series[timedelta64]+DatetimeIndex[tz] bugs
jbrockmendel Dec 20, 2017
2e824bc
set result name, add name checks to test
jbrockmendel Dec 21, 2017
5471d40
edits per reviewer request
jbrockmendel Dec 21, 2017
d5a0862
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 21, 2017
e142946
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 24, 2017
277b8cb
implement requested kludge
jbrockmendel Dec 26, 2017
5a5d98c
do names hapharzardly per request
jbrockmendel Dec 29, 2017
76582ac
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 29, 2017
5c2e883
whatsnew note
jbrockmendel Dec 29, 2017
ae3b34d
fix name mismatch from merge mangle
jbrockmendel Dec 29, 2017
e72bcf9
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 29, 2017
68727d4
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 29, 2017
dd2970c
handle names correctly
jbrockmendel Dec 29, 2017
26c9b51
check against ABC classes
jbrockmendel Dec 29, 2017
a7a3d6f
check against ABCDatetimeIndex
jbrockmendel Dec 29, 2017
3e71017
assert_produces_warning(PerformanceWarning)
jbrockmendel Dec 29, 2017
dead356
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 30, 2017
51234b3
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 31, 2017
7b831ff
requested convention edits
jbrockmendel Dec 31, 2017
c09ac0f
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Dec 31, 2017
0fc30bb
requested edit, extend tests to include ser.values
jbrockmendel Dec 31, 2017
ae1af46
dummy commit to force CI
jbrockmendel Jan 1, 2018
190197c
Merge branch 'master' of https://github.com/pandas-dev/pandas into id…
jbrockmendel Jan 1, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/source/whatsnew/v0.23.0.txt
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ Numeric
^^^^^^^

- Bug in :func:`Series.__sub__` subtracting a non-nanosecond ``np.datetime64`` object from a ``Series`` gave incorrect results (:issue:`7996`)
-
- Bug in :func:`Series.__add__` adding Series with dtype ``timedelta64[ns]`` to a timezone-aware ``DatetimeIndex`` incorrectly dropped timezone information (:issue:`13905`)
-

Categorical
Expand Down
8 changes: 6 additions & 2 deletions pandas/core/indexes/datetimelike.py
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,9 @@ def __add__(self, other):
from pandas.core.index import Index
from pandas.core.indexes.timedeltas import TimedeltaIndex
from pandas.tseries.offsets import DateOffset
if is_timedelta64_dtype(other):
if isinstance(other, ABCSeries):
return NotImplemented
elif is_timedelta64_dtype(other):
return self._add_delta(other)
elif isinstance(self, TimedeltaIndex) and isinstance(other, Index):
if hasattr(other, '_add_delta'):
Expand Down Expand Up @@ -697,7 +699,9 @@ def __sub__(self, other):
from pandas.core.indexes.datetimes import DatetimeIndex
from pandas.core.indexes.timedeltas import TimedeltaIndex
from pandas.tseries.offsets import DateOffset
if is_timedelta64_dtype(other):
if isinstance(other, ABCSeries):
return NotImplemented
elif is_timedelta64_dtype(other):
return self._add_delta(-other)
elif isinstance(self, TimedeltaIndex) and isinstance(other, Index):
if not isinstance(other, TimedeltaIndex):
Expand Down
3 changes: 3 additions & 0 deletions pandas/core/indexes/datetimes.py
Original file line number Diff line number Diff line change
Expand Up @@ -854,6 +854,9 @@ def _maybe_update_attributes(self, attrs):
return attrs

def _add_delta(self, delta):
if isinstance(delta, ABCSeries):
return NotImplemented

from pandas import TimedeltaIndex
name = self.name

Expand Down
31 changes: 21 additions & 10 deletions pandas/core/ops.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
from pandas.core.dtypes.generic import (
ABCSeries,
ABCDataFrame,
ABCIndex,
ABCIndex, ABCDatetimeIndex,
ABCPeriodIndex)

# -----------------------------------------------------------------------------
Expand Down Expand Up @@ -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?
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

values = values.to_series(keep_tz=True)
# datetime with tz
elif (isinstance(ovalues, datetime.datetime) and
hasattr(ovalues, 'tzinfo')):
Expand All @@ -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, pd.DatetimeIndex):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use ABC here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change.

# 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':
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggested a simplification

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i suggested a simplification

I'm guessing you're referring to this, which I responded to here.

Copy link
Contributor

Choose a reason for hiding this comment

The 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)
Expand Down
21 changes: 21 additions & 0 deletions pandas/tests/indexes/datetimes/test_arithmetic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 = pd.DatetimeIndex(['2016-06-28 05:30', '2016-06-28 05:31'],
tz=tz, name=names[0])
ser = pd.Series([pd.Timedelta(seconds=5)] * 2,
index=index, name=names[1])
expected = pd.Series(index + pd.Timedelta(seconds=5),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

convention is not to use pd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will change.

index=index, name=names[2])

# passing name arg isn't enough when names[2] is None
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you also test adding with ser.values as well (obviously will be index result). was part of the OP as an example.

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
Expand Down