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.

Sign up for GitHub

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

Fix Series[timedelta64]+DatetimeIndex[tz] bugs #18884

Show file tree
Hide file tree
Changes from 2 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
3 changes: 3 additions & 0 deletions pandas/core/indexes/datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
11 changes: 11 additions & 0 deletions pandas/core/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,17 @@ 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):
Copy link
Contributor

Choose a reason for hiding this comment

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

don't special case things here, this is the point of the _TimeOp class.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

#18832 is orthogonal.

What change have you indicated? Not clear on what "untangle this first" refers to.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 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.

# GH#13905
# Defer to DatetimeIndex/TimedeltaIndex operations where timezones
# are handled carefully.
result = op(pd.TimedeltaIndex(left), right)
name = _maybe_match_name(left, right)
result.name = name # in case name is None, needs to be overridden
return construct_result(left, result,
index=left.index, name=name,
dtype=result.dtype)
Copy link
Member Author

Choose a reason for hiding this comment

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

should name be passed using com._maybe_match_name here? Not sure what the convention is.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes


converted = _Op.get_op(left, right, name, na_op)

left, right = converted.left, converted.right
Expand Down
20 changes: 20 additions & 0 deletions pandas/tests/indexes/datetimes/test_arithmetic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,26 @@ 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.

res = ser + index
tm.assert_series_equal(res, expected)
Copy link
Contributor

Choose a reason for hiding this comment

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

pls use result=

res2 = index + ser
tm.assert_series_equal(res2, expected)


# GH 10699
@pytest.mark.parametrize('klass,assert_func', zip([Series, DatetimeIndex],
Expand Down