Skip to content

Fix DTI comparison with None, datetime.date #19301

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 17 commits into from
Feb 2, 2018
Merged

Fix DTI comparison with None, datetime.date #19301

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
b100f59
Fix DTI comparison with None, datetime.date
jbrockmendel Jan 18, 2018
59ac32c
add GH reference, whatsnew note
jbrockmendel Jan 18, 2018
79d9a02
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 20, 2018
6720de9
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 20, 2018
c2c1da5
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 21, 2018
69f527d
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 21, 2018
f2dbb7e
change return order per request
jbrockmendel Jan 21, 2018
0184b39
de-duplicate tests by giving proper names
jbrockmendel Jan 21, 2018
5dcc09c
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 22, 2018
78d4eb1
add nan to invalid test
jbrockmendel Jan 22, 2018
39ad04d
rename test_dti_cmp_invalid
jbrockmendel Jan 23, 2018
d578aba
add tests for comparisons against datetimelikes
jbrockmendel Jan 24, 2018
6ba5d99
requested name change
jbrockmendel Jan 24, 2018
578a4a3
requested edits
jbrockmendel Jan 25, 2018
b57d15d
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 26, 2018
0e0886a
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Jan 27, 2018
2ea1205
Merge branch 'master' of https://github.com/pandas-dev/pandas into dt…
jbrockmendel Feb 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
1 change: 1 addition & 0 deletions doc/source/whatsnew/v0.23.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,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`)
Copy link
Contributor

Choose a reason for hiding this comment

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

rebase again, I just updated


Indexing
^^^^^^^^
Expand Down
20 changes: 12 additions & 8 deletions pandas/core/indexes/datetimes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,18 @@ def wrapper(self, other):
else:
if isinstance(other, list):
other = DatetimeIndex(other)
elif not isinstance(other, (np.ndarray, Index, ABCSeries)):
other = _ensure_datetime64(other)
elif not isinstance(other, (np.datetime64, np.ndarray,
Index, ABCSeries)):
# Following Timestamp convention, __eq__ is all-False
# and __ne__ is all True, others raise TypeError.
if opname == '__eq__':
Copy link
Contributor

Choose a reason for hiding this comment

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

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

result = np.zeros(shape=self.shape, dtype=bool)
elif opname == '__ne__':
result = np.ones(shape=self.shape, dtype=bool)
else:
raise TypeError('%s type object %s' %
(type(other), str(other)))
return result

if is_datetimelike(other):
self._assert_tzawareness_compat(other)
Expand All @@ -147,12 +157,6 @@ def wrapper(self, other):
return compat.set_function_name(wrapper, opname, cls)


def _ensure_datetime64(other):
if isinstance(other, np.datetime64):
return other
raise TypeError('%s type object %s' % (type(other), str(other)))


_midnight = time(0, 0)


Expand Down
19 changes: 19 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 @@ -41,6 +41,25 @@ def addend(request):
return request.param


class TestDatetimeIndexComparisons(object):
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 lump the None with the NaT comparisons. Do we have the same testing of these comparisons for scalars?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@pytest.mark.parametrize('other', [None,
datetime(2016, 1, 1).date()])
def test_dti_cmp_invalid(self, tz, other):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

# GH#19301
dti = pd.date_range('2016-01-01', periods=2, tz=tz)
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@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').

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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


assert not (dti == other).any()
assert (dti != other).all()
with pytest.raises(TypeError):
dti < other
with pytest.raises(TypeError):
dti <= other
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

with pytest.raises(TypeError):
dti > other
with pytest.raises(TypeError):
dti >= other


class TestDatetimeIndexArithmetic(object):

def test_dti_add_timestamp_raises(self):
Expand Down