Fix DTI comparison with None, datetime.date #19301

jbrockmendel · 2018-01-18T17:21:09Z

Discussed in #19288

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

TomAugspurger · 2018-01-18T17:44:44Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+                                       datetime(2016, 1, 1).date()])
+    def test_dti_cmp_invalid(self, tz, other):
+        # GH#19301
+        dti = pd.date_range('2016-01-01', periods=2, tz=tz)


So is this saying that

DatetimeIndex([None, '2016-01-01']) == [None, datetime.date(2016, 1, 1)])

is [False, False]? I thought that in #18188 we decided that DatetimeIndex compared with datetime.date would coerce the date to a datetime at midnight?

@TomAugspurger I think part of the confusion is over Timestamp comparison vs DatetimeIndex comparison vs Series[datetime64] comparison (e.g. the whatsnew note in #18188 talks about Series (but puts the tests in index tests...)). This PR and a bunch of other recent ones have been focused on making the Index/Series behavior more consistent.

Following this, #19288 can be de-kludged to make Series[datetime64] comparison dispatch to DatetimeIndex comparison, ensuring internal consistency. But you're right that this would mean a change in the behavior of Series[datetime64] comparisons.

For the moment I'm taking Timestamp behavior as canonical and making DatetimeIndex match that.

ts = pd.Timestamp('2016-01-01') >>> ts == ts.to_pydatetime().date() False >>> ts < ts.to_pydatetime().date() Traceback (most recent call last): File "<stdin>", line 1, in <module> File "pandas/_libs/tslib.pyx", line 1165, in pandas._libs.tslib._Timestamp.__richcmp__ TypeError: Cannot compare type 'Timestamp' with type 'date'

I recall a discussion of whether date should be treated as datetime-at-midnight for Timestamp comparison purposes, my thought being it should be treated as Period(..., freq='D').

In [1]: ts = pd.Timestamp('2016-01-01') In [2]: ts Out[2]: Timestamp('2016-01-01 00:00:00') In [3]: ts.date() Out[3]: datetime.date(2016, 1, 1) In [4]: ts.to_pydatetime() Out[4]: datetime.datetime(2016, 1, 1, 0, 0) In [5]: ts.to_pydatetime() == ts.date() Out[5]: False In [6]: ts.to_pydatetime().date() == ts.date() Out[6]: True

I find [5] a bit odd.

I find [5] a bit odd.

The behavior is analogous to comparing Timestamp vs Period. eq and ne return False and True, respectively, and all others raise TypeError. Its odd if you interpret date as "datetime implicitly at midnight", but pretty intuitive if you interpret it as "less specific than a timestamp"

jreback · 2018-01-18T23:48:33Z

pandas/tests/indexes/datetimes/test_arithmetic.py

@@ -41,6 +41,25 @@ def addend(request):
    return request.param


+class TestDatetimeIndexComparisons(object):


can you lump the None with the NaT comparisons. Do we have the same testing of these comparisons for scalars?

AFAICT the comparison tests are scattered. I had planned to consolidate them here in a follow-up to keep narrow focus here.

can you lump the None with the NaT comparisons

They would need to be separate tests since the behavior is different, but I can put the tests adjacent to each other in this class.

Centralizing the DTI comparison tests is now a part of #19317. After that I'll make a pass to make sure all the relevant cases are covered.

jbrockmendel · 2018-01-19T23:32:15Z

Looks like Travis cancellation.

…i_cmp_fix

codecov · 2018-01-21T06:37:21Z

Codecov Report

Merging #19301 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #19301   +/-   ##
=======================================
  Coverage   91.67%   91.67%           
=======================================
  Files         148      148           
  Lines       48553    48553           
=======================================
  Hits        44513    44513           
  Misses       4040     4040

Flag	Coverage Δ
#multiple	`90.04% <100%> (ø)`	⬆️
#single	`41.71% <16.66%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/indexes/datetimes.py	`95.23% <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 35812ea...2ea1205. Read the comment docs.

jreback · 2018-01-21T15:28:11Z

rebase

…i_cmp_fix

jreback · 2018-01-21T17:53:09Z

pandas/core/indexes/datetimes.py

+                                        Index, ABCSeries)):
+                # Following Timestamp convention, __eq__ is all-False
+                # and __ne__ is all True, others raise TypeError.
+                if opname == '__eq__':


I think I prefer to return in the if/elif, then just raise instead of an else

jreback · 2018-01-21T17:56:46Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+            dti > other
+        with pytest.raises(TypeError):
+            dti >= other
+
    # TODO: De-duplicate with test_comparisons_nat below


is there a reason NOT to de-dupe with the other comparisons in this file (e.g. the TODO comment), in this PR? This is adding code then then will be inserted into the parameterization, I would rather just do it right once.

I think this comment was added in #19317, which was largely intended to be cut/paste with edits done in a separate PR. I can go and add this de-duplication to this PR, but that would be expanding the scope.

let's do it. right now these are a bit orphaned and should be combined with existing tests. in general this is a good thing to do anyhow.

Done. Turned out they weren't duplicated so much as poorly-named.

…i_cmp_fix

jreback · 2018-01-21T22:34:31Z

doc/source/whatsnew/v0.23.0.txt

@@ -434,6 +434,7 @@ Conversion
 - Bug in ``.astype()`` to non-ns timedelta units would hold the incorrect dtype (:issue:`19176`, :issue:`19223`, :issue:`12425`)
 - Bug in subtracting :class:`Series` from ``NaT`` incorrectly returning ``NaT`` (:issue:`19158`)
 - Bug in comparison of timezone-aware :class:`DatetimeIndex` against ``NaT`` incorrectly raising ``TypeError`` (:issue:`19276`)
+- Bug in comparison of :class:`DatetimeIndex` against ``None`` or ``datetime.date`` objects raising ``TypeError`` for ``==`` and ``!=`` comparisons instead of all-``False`` and all-``True``, respectively (:issue:`19301`)


rebase again, I just updated

jreback · 2018-01-21T22:35:41Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+        with pytest.raises(TypeError):
+            dti < other
+        with pytest.raises(TypeError):
+            dti <= other


this should also test pd.NaT here (and I think we have a tests for np.nan that should test raising).

Just added nan to the params for this test. pd.NaT test is immediately below this one.

the datetime.date part of this needs to move to where we test comparisions with Timestamp, datetime.datetime and np.datetime64

…i_cmp_fix

jreback · 2018-01-23T00:06:21Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+    @pytest.mark.parametrize('other', [None,
+                                       datetime(2016, 1, 1).date(),
+                                       np.nan])
+    def test_dti_cmp_invalid(self, tz, other):


hmm, I think leave None and np.nan here is ok (rename this from invalid -> null or something more descriptive).

put the date with a Timestamp / datetime test.

jbrockmendel · 2018-01-23T16:30:49Z

11 hours and the OSX build hasn't started. Possible problem with Travis?

jreback · 2018-01-24T01:27:32Z

see that red X on the status (on the travis page). They give messages.

jreback · 2018-01-24T01:29:14Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+        with pytest.raises(TypeError):
+            dti < other
+        with pytest.raises(TypeError):
+            dti <= other


the datetime.date part of this needs to move to where we test comparisions with Timestamp, datetime.datetime and np.datetime64

jbrockmendel · 2018-01-24T01:46:23Z

the datetime.date part of this needs to move to where we test comparisions with Timestamp, datetime.datetime and np.datetime64

OK, but it'll be a test identical the the one here, just in a different place.

jreback · 2018-01-24T02:09:50Z

my point is there already are tests for this - need to collocate them

jbrockmendel · 2018-01-24T03:00:54Z

my point is there already are tests for this - need to collocate them

OK. There are no such tests in this file; possible in scalar. My first choice is to not move anything in this PR, but if we have to either move DTI-with-timestamp-comparison test to tests.scalar or timestamp-with-DTI-comparison test to tests.indexes, I'd prefer the latter.

jbrockmendel · 2018-01-24T03:04:01Z

Not seeing it in scalar either. I guess I'll add some extras here.

jreback · 2018-01-24T11:14:10Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+            dti >= other
+
+    @pytest.mark.parametrize('other', [None,
+                                       np.nan])


can you add pd.NaT here

No, pd.NaT behaves differently from None or np.nan.

huh? that would be troubling, why is that

The __eq__ and __ne__ behave the same, but None and np.nan raise TypeError for the inequalities, whereas pd.NaT returns False.

right. ok then split the tests to reflect this, testing all nulls at once for eq/ne make the test easy to grok.

make the test easy to grok.

I'll do this, but object to the notions that fixtures and easy-to-grok can go in the same file. If I can't import the test and run it in the interpreter, then I'm in a Strange Land.

jreback · 2018-01-24T11:15:36Z

pandas/tests/indexes/datetimes/test_arithmetic.py

@@ -44,7 +45,74 @@ def addend(request):


 class TestDatetimeIndexComparisons(object):
-    # TODO: De-duplicate with test_comparisons_nat below
+    @pytest.mark.parametrize('other', [datetime(2016, 1, 1),


can you move all of these comparisons (that you are moving anyway) to a new test_compare.py

Are you sure? The other test_arithmetic.py files we've refactored out recently mostly include a TestComparison class.

I would like to separate them, a followup is ok.

jreback · 2018-01-24T11:16:53Z

pandas/tests/indexes/datetimes/test_arithmetic.py

+
+    @pytest.mark.parametrize('other', [None,
+                                       np.nan])
+    def test_dti_cmp_non_datetime(self, tz, other):


dti_cmp_null_scalar

…i_cmp_fix

jreback

looks ok. pls rebase

jreback · 2018-01-27T01:18:24Z

pandas/tests/indexes/datetimes/test_arithmetic.py

@@ -44,7 +45,74 @@ def addend(request):


 class TestDatetimeIndexComparisons(object):
-    # TODO: De-duplicate with test_comparisons_nat below
+    @pytest.mark.parametrize('other', [datetime(2016, 1, 1),


I would like to separate them, a followup is ok.

…i_cmp_fix

jbrockmendel · 2018-01-28T00:30:00Z

Thoughts here? This is now a blocker for the next steps in core.ops.

jreback · 2018-02-01T13:28:02Z

this looks good. rebase and ping on green.

…i_cmp_fix

jbrockmendel · 2018-02-01T22:37:02Z

ping

jreback · 2018-02-02T11:39:41Z

thanks!

@jbrockmendel just wanted to say. very much appreciate your changes. As they get more complicated and/or hit edge cases, I am necessarily being more of a stickler on things. don't take it personally (and you are very responsive!)

pandas is quite complicated and enforcing consistency across user AND developer experiences is hard, but very important.

jbrockmendel added 2 commits January 18, 2018 09:18

Fix DTI comparison with None, datetime.date

b100f59

add GH reference, whatsnew note

59ac32c

TomAugspurger reviewed Jan 18, 2018

View reviewed changes

jreback added Datetime Datetime data dtype Indexing Related to indexing on series/frames, not to indexes themselves Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Jan 18, 2018

jreback reviewed Jan 18, 2018

View reviewed changes

jbrockmendel mentioned this pull request Jan 19, 2018

WIP: Dispatch Series comparison methods to Index implementations #19288

Closed

jbrockmendel added 2 commits January 20, 2018 09:50

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

79d9a02

…i_cmp_fix

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

6720de9

…i_cmp_fix

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

c2c1da5

…i_cmp_fix

jreback requested changes Jan 21, 2018

View reviewed changes

jbrockmendel added 3 commits January 21, 2018 10:38

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

69f527d

…i_cmp_fix

change return order per request

f2dbb7e

de-duplicate tests by giving proper names

0184b39

jreback requested changes Jan 21, 2018

View reviewed changes

jbrockmendel added 2 commits January 21, 2018 16:23

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

5dcc09c

…i_cmp_fix

add nan to invalid test

78d4eb1

jreback requested changes Jan 23, 2018

View reviewed changes

rename test_dti_cmp_invalid

39ad04d

jreback requested changes Jan 24, 2018

View reviewed changes

add tests for comparisons against datetimelikes

d578aba

jreback requested changes Jan 24, 2018

View reviewed changes

requested name change

6ba5d99

jbrockmendel closed this Jan 25, 2018

requested edits

578a4a3

jbrockmendel reopened this Jan 25, 2018

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

b57d15d

…i_cmp_fix

jreback requested changes Jan 27, 2018

View reviewed changes

jreback added this to the 0.23.0 milestone Jan 27, 2018

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

0e0886a

…i_cmp_fix

This was referenced Jan 30, 2018

implement bits of numpy_helper in cython where possible #19450

Merged

Continue de-nesting core.ops #19448

Merged

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

2ea1205

…i_cmp_fix

jreback approved these changes Feb 2, 2018

View reviewed changes

jreback merged commit cd6510d into pandas-dev:master Feb 2, 2018

jbrockmendel deleted the dti_cmp_fix branch February 4, 2018 16:41

harisbal pushed a commit to harisbal/pandas that referenced this pull request Feb 28, 2018

Fix DTI comparison with None, datetime.date (pandas-dev#19301)

0160927

		@@ -41,6 +41,25 @@ def addend(request):
		return request.param


		class TestDatetimeIndexComparisons(object):

Fix DTI comparison with None, datetime.date #19301

Fix DTI comparison with None, datetime.date #19301

Conversation

jbrockmendel commented Jan 18, 2018 • 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

jbrockmendel commented Jan 19, 2018

codecov bot commented Jan 21, 2018 • edited Loading

Codecov Report

jreback commented Jan 21, 2018

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

jbrockmendel commented Jan 23, 2018

jreback commented Jan 24, 2018

Choose a reason for hiding this comment

jbrockmendel commented Jan 24, 2018

jreback commented Jan 24, 2018

jbrockmendel commented Jan 24, 2018

jbrockmendel commented Jan 24, 2018

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 28, 2018

jreback commented Feb 1, 2018

jbrockmendel commented Feb 1, 2018

jreback commented Feb 2, 2018

jbrockmendel commented Jan 18, 2018 •

edited

Loading

codecov bot commented Jan 21, 2018 •

edited

Loading