DEPR: disallow tznaive datetimes when indexing tzaware datetimeindex by jbrockmendel · Pull Request #36148 · pandas-dev/pandas (original) (raw)

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.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation12 Commits14 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

cc @mroeschke

Choose a reason for hiding this comment

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

IIRC there is an issue about this, and we had quite some discussion. I am +1 on actually doing this, but we might need to deprecate first. cc @jorisvandenbossche @TomAugspurger

+1 on doing this too. It would be great to do this for tz-naive datelike-strings as well.

IIRC there is an issue about this, and we had quite some discussion

would be great to do this for tz-naive datelike-strings as well.

The discussion 2ish years ago ended up deciding to special-case naive strings because a) user convenience and b) there's not a nice way to specify a tz in a string for e.g. "US/Eastern". IIRC the consensus was pretty specifically strings only, and the fact that we currently allow mismatched datetime/timestamp objects is the bug.

xref #18435, #17920, #18376, #24076

A more recent discussion about this is in #30994 (I think this PR is meant to address that issue?)

IIRC the consensus was pretty specifically strings only,

Yes, I think that is correct.
But that doesn't mean the current behaviour is a bug. It's maybe accidental, but nonetheless long-standing behaviour that we have been discussing for a long time (see all the elaborate discussions you are referencing), so it's clearly not that uncontroversial. So I agree with @jreback that we should do this with a deprecation cycle.
But to be clear: fully on board with the proposed change in behaviour.

the fact that we currently allow mismatched datetime/timestamp objects is the bug.

What do you mean exactly with "mismatched datetime/timestamp objects" (just the fact of tz-naive vs tz-aware)?

jorisvandenbossche changed the title~~BUG: allowing tznaive datetimes when indexing tzaware datetimeindex~~ API: disallow tznaive datetimes when indexing tzaware datetimeindex

Sep 7, 2020

@jorisvandenbossche so i have a way forward, is your suggestion that the two self._data._check_compatible_with calls this PR adds should be changed to something like

try:
    self._data._check_compatible_with(other)
except TypeError:
    warnings.warn("deprecated...", FutureWarning)

If so, is everyone else on board with that?

FWIW i think this behavior is sufficiently inconsistent we should call it a bugfix and be done with it, but I'm OK with deprecating if there's a consensus.

@jorisvandenbossche so i have a way forward, is your suggestion that the two self._data._check_compatible_with calls this PR adds should be changed to something like
try:
   self._data._check_compatible_with(other)
except TypeError:
   warnings.warn("deprecated...", FutureWarning)
If so, is everyone else on board with that?

FWIW i think this behavior is sufficiently inconsistent we should call it a bugfix and be done with it, but I'm OK with deprecating if there's a consensus.

+1 on this.

note we are deprecating for strings AND timestamps correct? (I think we should)

note we are deprecating for strings AND timestamps correct? (I think we should)

the discussion so far has been about changing/deprecating the incorrect timestamp behavior, not about undoing the special-casing for strings

updated as deprecation (though i still think this should be considered a bugfix)

jreback changed the title~~API: disallow tznaive datetimes when indexing tzaware datetimeindex~~ DEPR: disallow tznaive datetimes when indexing tzaware datetimeindex

Oct 2, 2020

lgtm, just resolved the conflict.

…das into dti-get_loc-awareness-2

kesmit13 pushed a commit to kesmit13/pandas that referenced this pull request

Nov 2, 2020

…andas-dev#36148)

BUG: allowing tznaive datetimes when indexing tzaware datetimeindex
isort fixup
deprecate wrong behavior
whatsnew

Co-authored-by: Jeff Reback jeff@reback.net

@jbrockmendel Should this be checked for insert too? Currently we are raising a FutureWarning for allow_duplicates=False because we run into if not allow_duplicates and column in self.columns:.

If this should raise I would either change the order of the calls here or explicitly check for the mismatch. If you remove this line, no warning is raised

So there's a case where frame.columns is tzaware and the user is doing frame.insert(int, tznaive_datetime, value)?

The __contains__ behavior is going to change for this user in once the deprecation is enforced, so they should see the warning right?

(or maybe a more specific warning?)

Labels

Deprecate

Functionality to remove in pandas

Indexing

Related to indexing on series/frames, not to indexes themselves

Timezones

Timezone data dtype