Fix+test timedelta64(nat) ops #23425

jbrockmendel · 2018-10-30T23:52:49Z

Fixes issues mentioned as follow-ups to #23320

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

pep8speaks · 2018-10-30T23:52:57Z

Hello @jbrockmendel! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/arrays/datetimelike.py !
There are no PEP8 issues in the file pandas/core/arrays/period.py !
There are no PEP8 issues in the file pandas/core/indexes/base.py !
There are no PEP8 issues in the file pandas/tests/arithmetic/test_datetime64.py !
There are no PEP8 issues in the file pandas/tests/arithmetic/test_period.py !
There are no PEP8 issues in the file pandas/tests/arithmetic/test_timedelta64.py !

codecov · 2018-10-31T00:33:27Z

Codecov Report

Merging #23425 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #23425      +/-   ##
==========================================
+ Coverage   92.25%   92.25%   +<.01%     
==========================================
  Files         161      161              
  Lines       51186    51195       +9     
==========================================
+ Hits        47222    47231       +9     
  Misses       3964     3964

Flag	Coverage Δ
#multiple	`90.64% <100%> (ø)`	⬆️
#single	`42.27% <23.07%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/base.py	`96.46% <ø> (ø)`	⬆️
pandas/core/arrays/datetimelike.py	`96.16% <100%> (+0.06%)`	⬆️
pandas/core/arrays/period.py	`98.09% <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 6bf6cd2...6d3d38d. Read the comment docs.

gfyoung · 2018-10-31T04:46:48Z

pandas/tests/arithmetic/test_datetime64.py

+        result = obj - other
+        tm.assert_equal(result, expected)
+        with pytest.raises(TypeError):
+            other - obj


I see a lot of error message changes. Are they tested anywhere with assert_raises_regex?

The way these tests are parametrized, the messages are all over the place.

jreback · 2018-10-31T11:59:55Z

pandas/core/arrays/datetimelike.py

@@ -382,6 +383,12 @@ def _add_timedeltalike_scalar(self, other):
        Add a delta of a timedeltalike
        return the i8 result view
        """
+        if isna(other):


isn't this pretty much what _nat_new did? I find this is repeating lots of code, why don't you make a method to create the null array instead.

_nat_new also did casting+boxing that this doesn't. These two lines aren't repeated often enough to merit a method.

jreback · 2018-10-31T12:03:29Z

pandas/core/arrays/period.py

@@ -757,7 +759,12 @@ def _add_timedeltalike_scalar(self, other):
        assert isinstance(self.freq, Tick)  # checked by calling function
        assert isinstance(other, (timedelta, np.timedelta64, Tick))

-        delta = self._check_timedeltalike_freq_compat(other)
+        if isna(other):


I find the need to do this pretty odd, why would't the check routine handle this? I do not like special cases

Because the first thing the check function does is call delta_to_nanoseconds, which will raise if we pass np.timedelta64("NaT"). Even if we change that method to avoid calling delta_to_nanoseconds in that case, we'd need to handle a null return value after the check.

I do not like special cases

+1. As Joris mentioned in another thread, a lot of gymnastics seems to be driven by the fact that pd.NaT does double-duty as both Not-A-Time and Not-A-Timedelta.

jreback · 2018-11-01T00:54:42Z

pandas/core/arrays/period.py

@@ -149,6 +149,8 @@ class PeriodArray(dtl.DatetimeLikeArrayMixin, ExtensionArray):
    period_array : Create a new PeriodArray
    pandas.PeriodIndex : Immutable Index for period data
    """
+    # array priority higher than numpy scalars
+    __array_priority__ = 1000


is there a test that needs this? I agree should set it....

Yes. IIRC it was timedelta64 - TimedeltaIndex

jreback · 2018-11-01T00:55:54Z

pandas/core/arrays/period.py

+        if isna(other):
+            # special handling for np.timedelta64("NaT"), avoid calling
+            #  _check_timedeltalike_freq_compat as that would raise TypeError
+            delta = other


can you write this like

if notna(other): # your current commnet other = self.........

jreback · 2018-11-01T00:56:47Z

pandas/tests/arithmetic/test_datetime64.py

@@ -1169,6 +1169,25 @@ def test_dti_isub_timedeltalike(self, tz_naive_fixture, two_hours):
        rng -= two_hours
        tm.assert_index_equal(rng, expected)

+    def test_dt64arr_add_sub_td64_nat(self, box, tz_naive_fixture):


can you parameterize other with pd.NaT as well here (I bet we already have a test for that, can you consolidate)

Not really; pd.NaT behavior is pretty starkly different from the behavior we’re testing here.

That said, there are several rounds of de duplication and parameterization coming up

how is it different at all?

Because this is a timedelta64 and pd.NaT is in most cases datetime-like. The "most cases" part makes it especially hairy.

jreback · 2018-11-01T00:57:37Z

pandas/tests/arithmetic/test_period.py

@@ -785,6 +785,24 @@ def test_pi_add_sub_timedeltalike_freq_mismatch_monthly(self,
        with tm.assert_raises_regex(period.IncompatibleFrequency, msg):
            rng -= other

+    def test_parr_add_sub_td64_nat(self, box):


same as above

jreback · 2018-11-01T00:57:47Z

pandas/tests/arithmetic/test_timedelta64.py

@@ -735,6 +735,24 @@ def test_td64arr_add_sub_tdi(self, box_df_broadcast_failure, names):
        else:
            assert result.dtypes[0] == 'timedelta64[ns]'

+    def test_td64arr_add_sub_td64_nat(self, box):
+        # GH#23320 special handling for timedelta64("NaT")


…gups2

jreback · 2018-11-06T13:24:21Z

lgtm. can you rebase. ping on green.

…gups2

Follow-up to pandas-devgh-23320.

jbrockmendel added 3 commits October 30, 2018 16:47

Fix+test timedelta64(nat) ops

fddcb14

flake8 fixup

57ec46a

flake8 fixups

f28f639

gfyoung added Bug Timedelta Timedelta data type Algos Non-arithmetic algos: value_counts, factorize, sorting, isin, clip, shift, diff labels Oct 31, 2018

gfyoung reviewed Oct 31, 2018

View reviewed changes

gfyoung added the Error Reporting Incorrect or improved errors from pandas label Oct 31, 2018

jreback requested changes Oct 31, 2018

View reviewed changes

jreback requested changes Nov 1, 2018

View reviewed changes

jbrockmendel added 2 commits October 31, 2018 18:59

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

8b5bf2c

…gups2

reverse isna/notna check

ed82665

jreback added this to the 0.24.0 milestone Nov 6, 2018

jreback approved these changes Nov 6, 2018

View reviewed changes

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

6d3d38d

…gups2

gfyoung merged commit d0691e0 into pandas-dev:master Nov 6, 2018

jbrockmendel deleted the bigups2 branch November 7, 2018 00:10

JustinZhengBC pushed a commit to JustinZhengBC/pandas that referenced this pull request Nov 14, 2018

Fix+test timedelta64(nat) ops (pandas-dev#23425)

278124e

Follow-up to pandas-devgh-23320.

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

Fix+test timedelta64(nat) ops (pandas-dev#23425)

3c92433

Follow-up to pandas-devgh-23320.

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Fix+test timedelta64(nat) ops (pandas-dev#23425)

c1a47f8

Follow-up to pandas-devgh-23320.

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Fix+test timedelta64(nat) ops (pandas-dev#23425)

6c5be6d

Follow-up to pandas-devgh-23320.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix+test timedelta64(nat) ops #23425

Fix+test timedelta64(nat) ops #23425

jbrockmendel commented Oct 30, 2018

pep8speaks commented Oct 30, 2018

codecov bot commented Oct 31, 2018 •

edited

Loading

gfyoung Oct 31, 2018

jbrockmendel Oct 31, 2018

jreback Oct 31, 2018

jbrockmendel Oct 31, 2018

jreback Nov 1, 2018

jreback Oct 31, 2018

jbrockmendel Oct 31, 2018

jreback Nov 1, 2018

jbrockmendel Nov 1, 2018

jreback Nov 1, 2018

jbrockmendel Nov 1, 2018

jreback Nov 1, 2018

jbrockmendel Nov 1, 2018

jreback Nov 1, 2018

jbrockmendel Nov 1, 2018

jreback Nov 1, 2018

jreback Nov 1, 2018

jreback commented Nov 6, 2018

Fix+test timedelta64(nat) ops #23425

Fix+test timedelta64(nat) ops #23425

Conversation

jbrockmendel commented Oct 30, 2018

pep8speaks commented Oct 30, 2018

codecov bot commented Oct 31, 2018 • 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

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 commented Nov 6, 2018

codecov bot commented Oct 31, 2018 •

edited

Loading