delegate (most) datetimelike Series arithmetic ops to DatetimeIndex #18817

jbrockmendel · 2017-12-18T04:51:26Z

DatetimeIndex arithmetic ops do overflow checks that Series does not. Instead of re-implementing those checks (which I tried and its a PITA) this PR just delegates the appropriate operations to the DatetimeIndex implementation.

There are a couple of things that are missing from the DatetimeIndex implementations, e.g. the int-array case added to indexes.datetimelike. If there's consensus that having a single implementation for these ops is the way to go, I'll port the other cases over in their own PRs.

If/when these implementations get merged, ops._TimeOp can be trimmed/removed.

closes Negative timedeltas for deltas > 292 years #12534
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2017-12-18T04:51:38Z

Hello @jbrockmendel! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 19, 2017 at 16:11 Hours UTC

codecov · 2017-12-18T05:38:38Z

Codecov Report

❗ No coverage uploaded for pull request base (master@07d8c2d). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #18817   +/-   ##
=========================================
  Coverage          ?    91.6%           
=========================================
  Files             ?      154           
  Lines             ?    51452           
  Branches          ?        0           
=========================================
  Hits              ?    47135           
  Misses            ?     4317           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`89.47% <100%> (?)`
#single	`40.8% <61.29%> (?)`

Impacted Files	Coverage Δ
pandas/core/ops.py	`90.56% <100%> (ø)`
pandas/core/indexes/datetimelike.py	`97.16% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07d8c2d...ab75018. Read the comment docs.

jreback · 2017-12-18T13:03:57Z

pandas/tests/series/test_timeseries.py

@@ -107,7 +107,7 @@ def test_shift(self):
        # incompat tz
        s2 = Series(date_range('2000-01-01 09:00:00', periods=5,
                               tz='CET'), name='foo')
-        pytest.raises(ValueError, lambda: s - s2)
+        pytest.raises(TypeError, lambda: s - s2)


this is an API change

as in needs to be noted in whatsnew or needs to be avoided?

jreback · 2017-12-18T13:05:19Z

pandas/core/ops.py

-            return all(isinstance(x, ABCDateOffset) for x in arr_or_obj)
-        return False
+
+def _is_offset(arr_or_obj):


better to move this to pandas.core.dtypes.common and de-privatize

and add tests

Agreed on all three points.

jreback · 2017-12-18T13:06:10Z

pandas/core/indexes/datetimelike.py

@@ -664,6 +665,12 @@ def __add__(self, other):
                return self.shift(other)
            elif isinstance(other, (Index, datetime, np.datetime64)):
                return self._add_datelike(other)
+            elif (isinstance(self, DatetimeIndex) and
+                  isinstance(other, np.ndarray) and other.size == 1 and


huh? I believe we already check is_integer_dtype(other). this check is much too specific

Under the status quo adding np.array(1, dtype=np.int64) to a DatetimeIndex behaves like adding Timedelta(nanosecond=1). Series raises (and a Series test fails unless this is caught here).

I'm open to ideas for how to make this less hacky.

An alternative would be to -- for the time being -- check for this case in the Series op and avoid dispatching to DatetimeIndex.

jreback · 2017-12-18T13:07:08Z

pandas/core/ops.py

@@ -715,6 +726,29 @@ def wrapper(left, right, name=name, na_op=na_op):
        if isinstance(right, ABCDataFrame):
            return NotImplemented

+        if _is_offset(right):


this logic does not belong here. this is what happens in Timeop.

this logic does not belong here. this is what happens in Timeop.

Yes. This is preventing this case from being dispatched to DatetimeIndex so that it continues to be handled by TimeOp for now. DatetimeIndex makes a mess of this case (try it...), but I decided to fix that separately.

…x12534

jreback

this needs some work. let's clear the decks and you can revist

jreback · 2017-12-23T20:52:12Z

doc/source/whatsnew/v0.22.0.txt

@@ -198,6 +198,7 @@ Other API Changes
 - Rearranged the order of keyword arguments in :func:`read_excel()` to align with :func:`read_csv()` (:issue:`16672`)
 - :func:`pandas.merge` now raises a ``ValueError`` when trying to merge on incompatible data types (:issue:`9780`)
 - :func:`wide_to_long` previously kept numeric-like suffixes as ``object`` dtype. Now they are cast to numeric if possible (:issue:`17627`)


move to 0.23.0

jbrockmendel · 2017-12-28T19:57:00Z

The other arithmetic PRs are a couple steps away from really being ready for this. The only part of this that is separately actionable (and on which I need input) is for where (and I guess if) to fix the mismatched behavior in DatetimeIndex.__add__ and __sub__

rng = pd.date_range('2000-01-01 09:00', freq='H', periods=2, tz='US/Pacific')
ser = pd.Series(rng)
other = np.array(1, dtype=np.int64)

>>> ser + other
[...]
TypeError: incompatible type for a datetime/timedelta operation [__add__]

>>> rng + other
DatetimeIndex(['2000-01-01 17:00:00.000000001-08:00', '2000-01-01 18:00:00.000000001-08:00'], dtype='datetime64[ns, US/Pacific]', freq=None)

>>> other + rng
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for +: 'numpy.ndarray' and 'DatetimeIndex'

This PR addressed that here and here, but got some pushback because it was (admittedly) ugly. Alternative suggestions requested.

Closing this PR, will (eventually) open another one with reduced scope.

delegate (most) datetimelike Series arithmetic ops to DatetimeIndex

8fb1a0d

flake8 fixup

6d7c6fb

jreback requested changes Dec 18, 2017

View reviewed changes

jbrockmendel added 2 commits December 18, 2017 07:33

Merge branch 'master' of https://github.com/pandas-dev/pandas into fi…

dd88a2c

…x12534

move+deprivatize+test is_offset

d86af39

jbrockmendel mentioned this pull request Dec 18, 2017

implement is_offsetlike #18823

Merged

4 tasks

gfyoung added Docs Timedelta Timedelta data type labels Dec 18, 2017

jbrockmendel added 6 commits December 18, 2017 09:49

fix test

bbea697

fixup typo

99e6cc6

Whatsnew note about change in exception class

61f22ba

move special case checking inside the is_datetime check

cf677a1

Merge branch 'master' of https://github.com/pandas-dev/pandas into fi…

7be6a97

…x12534

fixup name typo

ab75018

jreback requested changes Dec 23, 2017

View reviewed changes

jbrockmendel closed this Dec 28, 2017

jbrockmendel deleted the fix12534 branch February 11, 2018 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delegate (most) datetimelike Series arithmetic ops to DatetimeIndex #18817

delegate (most) datetimelike Series arithmetic ops to DatetimeIndex #18817

jbrockmendel commented Dec 18, 2017

pep8speaks commented Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 18, 2017 •

edited

Loading

jreback Dec 18, 2017

jbrockmendel Dec 18, 2017

jreback Dec 18, 2017

jreback Dec 18, 2017

jbrockmendel Dec 18, 2017

jreback Dec 18, 2017

jbrockmendel Dec 18, 2017

jbrockmendel Dec 18, 2017

jreback Dec 18, 2017

jbrockmendel Dec 18, 2017

jreback left a comment

jreback Dec 23, 2017

jbrockmendel commented Dec 28, 2017

delegate (most) datetimelike Series arithmetic ops to DatetimeIndex #18817

delegate (most) datetimelike Series arithmetic ops to DatetimeIndex #18817

Conversation

jbrockmendel commented Dec 18, 2017

pep8speaks commented Dec 18, 2017 • edited Loading

Comment last updated on December 19, 2017 at 16:11 Hours UTC

codecov bot commented Dec 18, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Dec 28, 2017

pep8speaks commented Dec 18, 2017 •

edited

Loading

codecov bot commented Dec 18, 2017 •

edited

Loading