DatetimeIndex get_loc validates date object

[knabben]

• closes BUG: Date objects cannot be compared against a DatetimeIndex #35466
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

Adding the date object try cast in get_loc.

[pep8speaks]

Hello @knabben! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-11-04 15:43:34 UTC

[jbrockmendel]

this is the right way to address #35466, but you also need to look at the other places where _recognized_scalars is used and make sure those tests are updated appropriately

[jbrockmendel]

Do we want dti == date to do partial-matching the same way we would with a date-only string? (i guess Id be OK punting on this)(

[knabben]

Do we want dti == date to do partial-matching the same way we would with a date-only string? (i guess Id be OK punting on this)(

@jbrockmendel feel free to catch this, looks like you have been working with the flow on GH#19800 and GH#19301.

But having dti == date, looks ok, since the date is timestamped after.

[simonjayhawkins]

@knabben can you add a release note to doc\source\whatsnew\v1.1.1.rst

[TomAugspurger]

It's not clear to me that the behavior on master is incorrect. Shouldn't we be following the behavior of Python's dict, which doesn't consider dates to be datetimes at midnight?

In [18]: d = {datetime.datetime(2000, 1, 1): 1}

In [19]: datetime.date(2000, 1, 1) in d
Out[19]: False

[knabben]

It's not clear to me that the behavior on master is incorrect. Shouldn't we be following the behavior of Python's dict, which doesn't consider dates to be datetimes at midnight?

In [18]: d = {datetime.datetime(2000, 1, 1): 1}

In [19]: datetime.date(2000, 1, 1) in d
Out[19]: False

I had the same question in the issue, maybe a date in DateTimeIndex should never be considered and this now should be the correct behavior, this is clear after adding, date in the recognized_scalars, started to break a lot of tests, that were not expecting this to be there (like blocks, internals, etc):

_recognized_scalars = (datetime, np.datetime64, date)

It was passing before because get_loc, was using a fallback for this translation.

cc @jbrockmendel

[jbrockmendel]

I think get_loc needs to match __eq__, which needs to match Timestamp.__eq__. I think ATM Timestamp doesn’t match stdlib datetime behavior, which id be OK with changing (following deprecation cycle)

…

On Fri, Jul 31, 2020 at 2:37 PM Amim Knabben ***@***.***> wrote: It's not clear to me that the behavior on master is incorrect. Shouldn't we be following the behavior of Python's dict, which doesn't consider dates to be datetimes at midnight? In [18]: d = {datetime.datetime(2000, 1, 1): 1} In [19]: datetime.date(2000, 1, 1) in dOut[19]: False I had the same question in the issue, maybe a date in DateTimeIndex should never be considered and this now should be the correct behavior, this is clear that after adding, *date*: _recognized_scalars = (datetime, np.datetime64, date) Break a lot of tests, that were not expecting this to be there (like blocks, internals, etc), it was passing because get_loc, was using a fallback for this translation. cc @jbrockmendel <https://github.com/jbrockmendel> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#35478 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB5UM6HQJM6Y2HNWY4JTKJDR6M2QLANCNFSM4PN7Q52Q> .

[knabben]

closing the PR for the lack of time to investigate it further, for those going on:

1. It should be keep the behavior of date in DatetimeIndex equivalence, at least while in the deprecation cycle.
2. For the long run assess if this behavior still makes sense, deprecating otherwise.

[jorisvandenbossche]

It's not clear to me that the behavior on master is incorrect. Shouldn't we be following the behavior of Python's dict, which doesn't consider dates to be datetimes at midnight?

I think get_loc needs to match __eq__, which needs to match Timestamp.__eq__.

Indeed, right now, Timestamp considers a date to be equal:

In [33]: pd.Timestamp("2012-01-01") == datetime.date(2012, 1, 1)  
Out[33]: True

So I think if we want to change this to False, that should go through a deprecation cycle, and the same for the indexing behaviour.

[jorisvandenbossche]

Although that for Index/Array, we return False ...

In [34]: pd.array([pd.Timestamp("2012-01-01")]) == datetime.date(2012, 1, 1) 
Out[34]: array([False])

In [35]: pd.Index([pd.Timestamp("2012-01-01")]) == datetime.date(2012, 1, 1) 
Out[35]: array([False])

[dwardzinski-jsf]

Just tested this PR to see if it would fix the original issue, it looks like it still doesn't :(

>>> datetime.date(2000,1,1) in pd.DatetimeIndex(['2000-01-01'])
False

[jbrockmendel]

Indeed, right now, Timestamp considers a date to be equal:

It also looks like Timestamp fails to raise TypeError when doing checking Timestamp[aware] < date.

Also looks like Timestamp(not_midnight) == date returns True because of NotImplemented semantics

[jreback]

@knabben can you merge master

[jbrockmendel]

-1, DTI.get_loc should raise on a date object. xref #36131

[jreback]

-1, DTI.get_loc should raise on a date object. xref #36131

good point

thanks for the effort @knabben love to have you contribute on another issue!