Fix Timedelta.floordiv, rfloordiv #18961

jbrockmendel · 2017-12-27T21:58:49Z

It would not at all surprise me to learn that there are more corner cases that this misses. Needs some eyeballs.

closes BUG: Timedelta.__floordiv__ - need historical context #18846
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

jreback · 2017-12-28T01:49:44Z

pandas/tests/scalar/test_timedelta.py

+        assert (2 * td) // scalar.to_timedelta64() == 2
+
+        assert td // np.nan is pd.NaT
+        assert np.isnan(td // pd.NaT)


these should prob be in test_nat with other nat tests

TestTimedeltaArithmetic doesn't have a dedicated test/section for nat tests.

pandas/tests/scalar/test_nat is where the nat tests go (i don't mind them here), but should have some (even if slightly duplicative)

jreback · 2017-12-28T01:50:37Z

pandas/_libs/tslibs/timedeltas.pyx

+                # also timedelta-like
+                # We need to watch out for np.timedelta64('NaT')
+                if np.ndim(other) == 0:
+                    if np.isnat(other):


pls try to be DRY
don’t use np.ndim ; we don’t use it anywhere else
don’t use np.isnat

don’t use np.isnat

This is specifically trying to catch np.timedelta64('NaT') and because of np.NaT rules we can't use is timedelta64('NaT') and we can't use == timedelta64('NaT')

jreback · 2017-12-28T01:52:18Z

pandas/_libs/tslibs/timedeltas.pyx


+        if isinstance(other, float) and np.isnan(other):


huh?
why are you not just checking isna

Because np.nan (which can't be detected with other is np.nan) gets treated differently from pd.NaT.

why? nan is equiv of nat here

why? nan is equiv of nat here

That's slightly ambiguous, but I lean towards "no it isn't". Timedelta(nan) does return NaT, but Timedelta / float is meaningful on its own, so we should treat nan as a float for the purposes of this operation.

checknull(other) and is_float_object(other) is more clear here (and add a comment)

jreback · 2017-12-28T01:53:15Z

pandas/_libs/tslibs/timedeltas.pyx

        elif not _validate_ops_compat(other):
            return NotImplemented

        other = Timedelta(other)
        if other is NaT:
-            return NaT
+            return np.nan


is this what we do for division?
is it not tested now?

is this what we do for division?

Yes.

The status quo is... weird.

def __floordiv__(self, other): if hasattr(other, 'dtype'): # work with i8 other = other.astype('m8[ns]').astype('i8') return self.value // other elif is_integer_object(other): # integers only return Timedelta(self.value // other, unit='ns') elif not _validate_ops_compat(other): return NotImplemented other = Timedelta(other) if other is NaT: return np.nan return self.value // other.value def __rfloordiv__(self, other): if hasattr(other, 'dtype'): # work with i8 other = other.astype('m8[ns]').astype('i8') return other // self.value elif not _validate_ops_compat(other): return NotImplemented other = Timedelta(other) if other is NaT: return NaT return other.value // self.value

is it not tested now?

Not that I'm aware of.

jreback · 2017-12-28T01:54:53Z

pandas/_libs/tslibs/timedeltas.pyx

-            other = other.astype('m8[ns]').astype('i8')
-            return other // self.value
+            if other.dtype.kind == 'm':
+                # also timedelta-like


iirc for division we defer to. TDI to handle his logic
does this code path follow?

iirc for division we defer to. TDI to handle his logic

I would find that design choice surprising.

def __truediv__(self, other): if hasattr(other, 'dtype'): return self.to_timedelta64() / other elif is_integer_object(other) or is_float_object(other): # integers or floats return Timedelta(self.value / other, unit='ns') elif not _validate_ops_compat(other): return NotImplemented other = Timedelta(other) if other is NaT: return np.nan return self.value / float(other.value)

I think the relevant difference here is for cases where other is an array this returns NotImplemented, but that isn't a viable option for floordiv because numpy doesn't implement floordiv for timedelta64.

…_floordiv

codecov · 2017-12-28T19:46:15Z

Codecov Report

Merging #18961 into master will decrease coverage by 49.91%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master   #18961       +/-   ##
===========================================
- Coverage   91.53%   41.61%   -49.92%     
===========================================
  Files         148      148               
  Lines       48688    48754       +66     
===========================================
- Hits        44566    20289    -24277     
- Misses       4122    28465    +24343

Flag	Coverage Δ
#multiple	`?`
#single	`41.61% <ø> (-0.02%)`	⬇️

Impacted Files	Coverage Δ
pandas/tseries/converter.py	`0% <0%> (-100%)`	⬇️
pandas/lib.py	`0% <0%> (-100%)`	⬇️
pandas/tools/hashing.py	`0% <0%> (-100%)`	⬇️
pandas/tslib.py	`0% <0%> (-100%)`	⬇️
pandas/parser.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas_constants.py	`0% <0%> (-100%)`	⬇️
pandas/types/concat.py	`0% <0%> (-100%)`	⬇️
pandas/io/sas/sas7bdat.py	`0% <0%> (-91.08%)`	⬇️
pandas/io/sas/sas_xport.py	`0% <0%> (-90.28%)`	⬇️
pandas/core/tools/numeric.py	`10.44% <0%> (-89.56%)`	⬇️
... and 109 more

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 e1888f0...f101378. Read the comment docs.

…_floordiv

jbrockmendel · 2017-12-29T06:45:21Z

The linting doesn't like np.testing.assert_array_equal. What is the preferred usage?

jreback · 2017-12-29T19:04:19Z

we have banned all numpy functions, e.g. np.testing.assert_array_equal instead use tm.assert_numpy_array_equal; they are not generally flexibile enough and some are buggy (don't compare object arrays correctly for example)

…_floordiv

jreback

pls acknowledge comments individually (generally)
i don’t want to repeat myself

jreback · 2017-12-31T00:59:30Z

pandas/_libs/tslibs/timedeltas.pyx


+        if isinstance(other, float) and np.isnan(other):


why? nan is equiv of nat here

jreback · 2017-12-31T01:01:05Z

pandas/_libs/tslibs/timedeltas.pyx

+
+    # We need to watch out for np.timedelta64('NaT')
+    if ndim == 0:
+        if np.isnat(other):


It looks like the Travis build with numpy 1.10.4 is failing because there is no np.isnat, so this is necessary even aside from matters of taste/convention.

I'm making the suggested change now, but is there an alternative that doesn't involve reaching into non-cython pandas? I have a strong preference for keeping the dependency graph DAG.

you mean like missing.pyx/is_null_datetimelike

jreback · 2017-12-31T01:01:12Z

pandas/_libs/tslibs/timedeltas.pyx

+            return operation(value, other.astype('m8[ns]').astype('i8'))
+
+    else:
+        mask = np.isnat(other)


…_floordiv

jreback · 2018-01-05T19:27:35Z

pandas/_libs/tslibs/timedeltas.pyx

+                else:
+                    return self.to_timedelta64() // other
+
+            else:


just raise you don't need an else

jreback · 2018-01-05T19:27:57Z

pandas/_libs/tslibs/timedeltas.pyx

+            elif other.dtype.kind in ['i', 'u', 'f']:
+                if np.ndim(other) == 0:
+                    return Timedelta(self.value // other)
+                else:


you don't need the else

also pls use other.ndim

jreback · 2018-01-05T19:28:13Z

pandas/_libs/tslibs/timedeltas.pyx

+            if other.dtype.kind == 'm':
+                # also timedelta-like
+                return _broadcast_floordiv_td64(self.value, other, _rfloordiv)
+            else:


jreback · 2018-01-05T19:28:45Z

pandas/_libs/tslibs/timedeltas.pyx


+        if isinstance(other, float) and np.isnan(other):


checknull(other) and is_float_object(other) is more clear here (and add a comment)

jreback · 2018-01-05T19:29:23Z

pandas/_libs/tslibs/timedeltas.pyx

+
+    # We need to watch out for np.timedelta64('NaT').
+    mask = other.view('i8') == NPY_NAT
+    # equiv np.isnat, which does not exist in some supported np versions.


remove this comment, we DO NOT use np.isnat

jreback · 2018-01-05T19:29:33Z

pandas/_libs/tslibs/timedeltas.pyx

+    if ndim == 0:
+        if mask:
+            return np.nan
+        else:


jreback · 2018-01-05T19:29:40Z

pandas/_libs/tslibs/timedeltas.pyx

+        else:
+            return operation(value, other.astype('m8[ns]').astype('i8'))
+
+    else:


Will change, but curious why you have such a strong preference here. I tend to like making these elses explicit (you've no doubt noticed).

jreback · 2018-01-05T19:30:30Z

pandas/tests/scalar/test_timedelta.py

+        assert (2 * td) // scalar.to_timedelta64() == 2
+
+        assert td // np.nan is pd.NaT
+        assert np.isnan(td // pd.NaT)


pandas/tests/scalar/test_nat is where the nat tests go (i don't mind them here), but should have some (even if slightly duplicative)

…_floordiv

jreback

test comments, lgtm, ping on green.

jreback · 2018-01-06T17:44:42Z

pandas/tests/scalar/test_timedelta.py

+
+        dt64 = np.datetime64('2016-01-01', dtype='datetime64[us]')
+        with pytest.raises(TypeError):
+            td.__rfloordiv__(dt64)


adding a bit more comments here enumerating the cases would be useful

jbrockmendel · 2018-01-07T01:51:23Z

Ping

jreback · 2018-01-07T02:40:37Z

thanks!

Fix Timedelta.__floordiv__, __rfloordiv__

71c6835

jreback requested changes Dec 28, 2017

View reviewed changes

jreback added Bug Timedelta Timedelta data type labels Dec 28, 2017

jbrockmendel added 2 commits December 28, 2017 11:00

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

3699934

…_floordiv

Whatsnew note, DRY off a bit

c031864

jbrockmendel added 2 commits December 28, 2017 20:34

lint pickiness

09903b4

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

a2d6dc9

…_floordiv

use tm.assert_numpy_array_equal

54cc35f

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

599e50f

…_floordiv

jreback requested changes Dec 31, 2017

View reviewed changes

jbrockmendel mentioned this pull request Dec 31, 2017

BUG: Timedelta.__floordiv__ - need historical context #18846

Closed

jbrockmendel added 6 commits January 4, 2018 09:46

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

13a5a20

…_floordiv

avoid np.isnat

0e532fc

make dtype explcit to fix appveyor test error

79c3a8e

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

6af6ee4

…_floordiv

fix appveyor int32

a0029ad

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

c0f4783

…_floordiv

jreback requested changes Jan 5, 2018

View reviewed changes

jbrockmendel added 3 commits January 5, 2018 13:03

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

0401851

…_floordiv

requested edits

3c38971

fix copy/paste screwup

3501956

jreback requested changes Jan 6, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Jan 6, 2018

requested comments

f101378

jreback approved these changes Jan 7, 2018

View reviewed changes

jreback merged commit 36a71eb into pandas-dev:master Jan 7, 2018

jbrockmendel mentioned this pull request Jan 7, 2018

DataFrame vs Series vs Index arithmetic Roundup #18824

Closed

59 tasks

jbrockmendel deleted the td_floordiv branch January 23, 2018 04:40

jorisvandenbossche mentioned this pull request Feb 19, 2018

BUG: regression in Timedelta.__rfloordiv__ with integer data #19761

Closed

Fix Timedelta.__floordiv__, __rfloordiv__ #18961

Fix Timedelta.__floordiv__, __rfloordiv__ #18961

Conversation

jbrockmendel commented Dec 27, 2017 • edited Loading

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

codecov bot commented Dec 28, 2017 • edited Loading

Codecov Report

jbrockmendel commented Dec 29, 2017

jreback commented Dec 29, 2017

jreback left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbrockmendel commented Jan 7, 2018

jreback commented Jan 7, 2018

Fix Timedelta.floordiv, rfloordiv #18961

Fix Timedelta.floordiv, rfloordiv #18961

jbrockmendel commented Dec 27, 2017 •

edited

Loading

codecov bot commented Dec 28, 2017 •

edited

Loading