Repository: pandas-dev/pandas
Pull request: Fix Series[timedelta64]+DatetimeIndex[tz] bugs
ser + index lost timezone
index + ser retained timezone but returned a DatetimeIndex

- [x] closes #13905
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry


Problem statement:
DatetimeIndex + TimeDelta gives wrong results when timezone is set
When I add a time delta to a DatetimeIndex which is tz-aware, the results appear as UTC, but sometimes claim to be in the original non-UTC timezone.
#### Code

```
import pandas as pd
index = pd.DatetimeIndex(['2016-06-28 05:30', '2016-06-28 05:31'], dtype='datetime64[ns, America/Chicago]')
offset = pd.Series(['00:00:05', '00:00:05'], dtype='timedelta64[ns]', index=index)
print(index + offset)
print(index + offset.values)
```
#### Actual Output

```
2016-06-28 05:30:00-05:00   2016-06-28 10:30:05
2016-06-28 05:31:00-05:00   2016-06-28 10:31:05
dtype: datetime64[ns]
DatetimeIndex(['2016-06-28 10:30:05-05:00', '2016-06-28 10:31:05-05:00'], dtype='datetime64[ns, America/Chicago]', freq=None)
```
#### Expected Output

```
2016-06-28 05:30:00-05:00   2016-06-28 05:30:05-05:00
2016-06-28 05:31:00-05:00   2016-06-28 05:31:05-05:00
dtype: datetime64[ns]
DatetimeIndex(['2016-06-28 05:30:05-05:00', '2016-06-28 05:31:05-05:00'], dtype='datetime64[ns, America/Chicago]', freq=None)
```

Note however that this does print the expected result:

```
print(index + pd.to_timedelta('00:00:05'))
```

I'm using Pandas 0.18.1.

Hints from issue discussion:
yep, thought we had an issue about this, but can't seem to find it.

Commit message: Fix Series[timedelta64]+DatetimeIndex[tz] bugs

ser + index lossed timezone
closes #13905

index + ser retained timezone but returned a DatetimeIndex

Patch to review:
diff --git a/pandas/core/indexes/datetimes.py b/pandas/core/indexes/datetimes.py
index ec5c20d341b50..860e17db0bf45 100644
--- a/pandas/core/indexes/datetimes.py
+++ b/pandas/core/indexes/datetimes.py
@@ -855,6 +855,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
 
diff --git a/pandas/core/ops.py b/pandas/core/ops.py
index e23609b23f529..fec263b1f4ec5 100644
--- a/pandas/core/ops.py
+++ b/pandas/core/ops.py
@@ -709,6 +709,15 @@ def wrapper(left, right, name=name, na_op=na_op):
 
         left, right = _align_method_SERIES(left, right)
 
+        if is_timedelta64_dtype(left) and isinstance(right, pd.DatetimeIndex):
+            # GH#13905
+            # Defer to DatetimeIndex/TimedeltaIndex operations where timezones
+            # are handled carefully.
+            result = op(pd.TimedeltaIndex(left), right)
+            return construct_result(left, result,
+                                    index=left.index, name=left.name,
+                                    dtype=result.dtype)
+
         converted = _Op.get_op(left, right, name, na_op)
 
         left, right = converted.left, converted.right
diff --git a/pandas/tests/indexes/datetimes/test_arithmetic.py b/pandas/tests/indexes/datetimes/test_arithmetic.py
index a46462e91a866..fcf38629b5815 100644
--- a/pandas/tests/indexes/datetimes/test_arithmetic.py
+++ b/pandas/tests/indexes/datetimes/test_arithmetic.py
@@ -363,6 +363,21 @@ def test_datetimeindex_sub_timestamp_overflow(self):
             with pytest.raises(OverflowError):
                 dtimin - variant
 
+    @pytest.mark.parametrize('tz', [None, 'America/Chicago'])
+    def test_dti_add_series(self, tz):
+        # GH#13905
+        index = pd.DatetimeIndex(['2016-06-28 05:30', '2016-06-28 05:31'],
+                                 tz=tz)
+        ser = pd.Series([pd.Timedelta(seconds=5)] * 2,
+                        index=index)
+        expected = pd.Series(index + pd.Timedelta(seconds=5),
+                             index=index)
+        assert expected.dtype == index.dtype
+        res = ser + index
+        tm.assert_series_equal(res, expected)
+        res2 = index + ser
+        tm.assert_series_equal(res2, expected)
+
 
 # GH 10699
 @pytest.mark.parametrize('klass,assert_func', zip([Series, DatetimeIndex],


Code review comments:
diff_hunk: @@ -709,6 +709,15 @@ def wrapper(left, right, name=name, na_op=na_op):
 
         left, right = _align_method_SERIES(left, right)
 
+        if is_timedelta64_dtype(left) and isinstance(right, pd.DatetimeIndex):
+            # GH#13905
+            # Defer to DatetimeIndex/TimedeltaIndex operations where timezones
+            # are handled carefully.
+            result = op(pd.TimedeltaIndex(left), right)
+            return construct_result(left, result,
+                                    index=left.index, name=left.name,
+                                    dtype=result.dtype)
path: pandas/core/ops.py
text: @author:
should `name` be passed using `com._maybe_match_name` here?  Not sure what the convention is.

@user1:
yes

diff_hunk: @@ -709,6 +709,15 @@ def wrapper(left, right, name=name, na_op=na_op):
 
         left, right = _align_method_SERIES(left, right)
 
+        if is_timedelta64_dtype(left) and isinstance(right, pd.DatetimeIndex):
path: pandas/core/ops.py
text: @user1:
don't special case things *here*, this is the point of the _TimeOp class.

@author:
This is the appropriate place.  Adding additional wrapping/unwrapping in _TimeOp obscures whats going on.  Even if it wasn't a third(!) level of wrapping, b/c of namespacing/closure issues it is essentially impossible to handle overflow checks correctly with the current setup.

Eventually this is going to _have_ to look like:

```
def wrapper(...)
    if isinstance(right, ABCDataFrame):
        return NotImplemented
    elif is_timedelta64_dtype(left):
         [dispatch to TimedeltaIndex op]
    elif (is_datetime64_dtype(left) or is_datetime64tz_dtype(left)):
         [dispatch to DatetimeIndex op]

    [everything else]
```

Everything _TimeOp does is done (better) in the index classes.  Anything other than dispatching to those methods is unnecessary duplication and an invitation to inconsistency.


@user1:
so let's untangle that first. This bug fix just addsmore things to undo later.

@author:
> This bug fix just addsmore things to undo later.

This won't need to be undone.  Eventually the `and isinstance(right, pd.DatetimeIndex)` part of `if is_timedelta64_dtype(left) and isinstance(right, pd.DatetimeIndex):` is removed and the timedelta64_dtype case is complete.

I've given the order of edits quite a bit of thought.  Transitioning towards the dispatch approach case-by-case and implementing corresponding tests along the way is the way to go.

> so let's untangle that first.

Untangle which first?  The mess of closures that prevents overflow checks?

I guess if you want to untangle _TimeOp independently, #18832 is a step in that direction, is orthogonal to the other outstanding PRs.


@user1:
well, until #18832 this needs to change as I have indicated.

@author:
#18832 is orthogonal.

What change have you indicated? Not clear on what "untangle this first" refers to.

@user1:
well I don't want this here. put it where the other conversions are. that can be reactored at some point if its worthile.

@author:
I guess if I squint it kind of makes sense to avoid fixing the overflow-check bug in this PR and instead do it separately.  Will change.