implement assert_tzawareness_compat for DatetimeIndex
State: closed

closes #18162 

ATM comparing tzaware `DatetimeIndex` with tznaive DTI fails to raise.  This PR implements `_assert_tzawareness_compa`t (which currently exists in `_Timestamp`) in `DatetimeIndex` to fix that.

That in turn causes breakage in `Series.replace`.  There's a small edit in `core.internals` to fix that, but I'm not sure that's the best way to make that work.

There is still one remaining test error in `tests.series.test_indexing.TestSeriesIndexing.test_getitem_setitem_datetimeindex`, which may either represent a bug or a case where tznaive/tzaware rules are relaxed.

```
N = 50
rng = date_range('1/1/1990', periods=N, freq='H', tz='US/Eastern')
ts = Series(np.random.randn(N), index=rng)

lb = "1990-01-01 04:00:00"
rb = "1990-01-01 07:00:00"

>>> result = ts[(ts.index >= lb) & (ts.index <= rb)]
TypeError: Cannot compare tz-naive and tz-aware datetime-like objects
```

Later in the same tests it tries to compare `ts.index` to naive `datetime` objects.  Is this a special case where we are intentionally less strict?

Labels:
name: Bug
name: Timezones
description: Timezone data dtype

Comments:
#17920 makes some desig decisions about tzawareness strictness.   Conditional on those decisions, comparisons should probably use the same conventions.
#17920 is orthogonal. That is purely string based. When you use Timestamps, they *must* be the same timezone (or None) or should raise 
> #17920 is orthogonal. That is purely string based. When you use Timestamps, they must be the same timezone (or None) or should raise

See the OP.  The unresolved error here is in comparing a tzaware DatetimeIndex against naive-like strings.


```
rng = pd.date_range('1/1/1990', periods=50, freq='H', tz='US/Eastern')
ts = pd.Series(np.random.randn(50), index=rng)

lb = "1990-01-01 04:00:00"
rb = "1990-01-01 07:00:00"
```

Under the status quo, the following are equivalent:

```
ts[lb:rb]
ts[(ts.index >= lb) & (ts.index <= rb)]
```

#17920 affects the behavior of `ts[lb:rb]` (though the output is unchanged in this case) while this affects the behavior of `ts.index >= lb`.  If we fix the comparison issue and implement #17920, we break the equivalence of slicing methods.  Which may be acceptable; I just want to be explicit about it.
@jbrockmendel Honestly I used some parts of pandas for my project and focussed on that part but I do not have deep insight into the internals and wherelse the changes I suggested could be made. Your comparison seems like an edge case to me but people have different tasks to do with this great library. I would rather go for the split between naive and timezoned datetime-like objects and my `UserWarning` instead of an error was rather for backwards-compability. Whenever we mix naive and timezoned things, it gets nasty. In my opinion assuming UTC is not good and should be done explicitely by the programmers.
Increasingly I think we should be strict about this, i.e. be Technically Correct.  Anything else is going to require keeping track of what rules are loosened in which special cases, and will inevitably lead to headaches down the road.

That said, an idea for a workaround for slicing tzaware indexes: interpret a trailing "TZ" at the end of a string as "interpret this naive datetime-like string with this DatetimeIndex's timezone".

Is #17920 motivated by a pressing use case where you've got a tzaware DatetimeIndex?  I expect the large majority of indexing use cases are tznaive.
It is motivated by a use case with data from several different data sources. Some are UTC, some are German Winter Time (without daylight saving time, definitely not a standard time), some are CET etc. I believe that mixing different data sources often has timezone issues as a consequence. But a prove I can not serve.

Adding a `TZ` is not an option in ISO 8601 as far as I have read it. And I would try to comply to that standard for the labels. Your attempt sounds quite experimential to me.
Let's move the discussion over to #18435.  Closing this PR until the design issue is resolved.
Reopening.  If we're going to introduce inconsistencies, might as well go full-speed ahead and get them sorted out sooner rather than later.  Will push update shortly.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=h1) Report
> Merging [#18376](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/aec33479b923a05ead1ca35335f00aba87e4145e?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `95.83%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18376/graphs/tree.svg?height=150&width=650&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #18376      +/-   ##
==========================================
- Coverage   91.36%   91.34%   -0.02%     
==========================================
  Files         163      163              
  Lines       49704    49723      +19     
==========================================
+ Hits        45411    45420       +9     
- Misses       4293     4303      +10
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.14% <95.83%> (ø)` | :arrow_up: |
| #single | `39.66% <33.33%> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.54% <100%> (+0.05%)` | :arrow_up: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `94.44% <90%> (-0.03%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.8% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=footer). Last update [aec3347...0f2287c](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=h1) Report
> Merging [#18376](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/f2d8db1acccd73340988af9ad5874252fd5c3967?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/18376/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #18376      +/-   ##
==========================================
+ Coverage   91.53%   91.53%   +<.01%     
==========================================
  Files         148      148              
  Lines       48688    48701      +13     
==========================================
+ Hits        44566    44579      +13     
  Misses       4122     4122
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.9% <100%> (ø)` | :arrow_up: |
| #single | `41.63% <71.42%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/18376/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.23% <100%> (+0.05%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=footer). Last update [f2d8db1...31b7336](https://codecov.io/gh/pandas-dev/pandas/pull/18376?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

> this is not the place for this, might need to fixe .replace in the block itself.

The pertinent call doesn't go through Block.replace.  NDFrame.replace calls BlockManager.replace_list.  Maybe edit NDFrame.replace to call Block.replace and add the appropriate check to DatetimeBlock.replace?  Whatever we can do to limit the scope...
If we don't want to special-case the `comp` function, then we could dispatch to `b.replace` in like in the `b.dtype == np.object_` case, then override `replace` for `DatetimeTZBlock`.  Is that what you have in mind?

so ``replace_list`` is a giant hack ATM and needs to be fixed. maybe this is the impetus. However, don't really want to add your hack on top. What exactly is failing w/o you touching replace? let's isolate and x-fail those tests for now.
> so replace_list is a giant hack ATM and needs to be fixed. maybe this is the impetus. However, don't really want to add your hack on top. What exactly is failing w/o you touching replace? let's isolate and x-fail those tests for now.

I like this plan, I'm [excited to be a part of it](https://www.youtube.com/watch?v=PVhTDNlbsSc).

In tests.indexing.test_coercion.TestReplaceSeriesCoercion.test_replace_series_datetime64tz, we have:

```
index = pd.Index([3, 4], name='xxx')
data = [Timestamp('2011-01-01 00:00:00-0500', tz='US/Eastern'), Timestamp('2011-01-03 00:00:00-0500', tz='US/Eastern')]
obj = pd.Series(data, index=index, name='yyy')
replacer = {Timestamp('2011-01-01 00:00:00-0500', tz='US/Eastern'): 1.1, Timestamp('2011-01-03 00:00:00-0500', tz='US/Eastern'): 2.2}

>>> obj.replace(replacer)  # <-- raises
```

When it gets to `replace_list`, it calls `asm8` on those `Timestamps`, which drops their tz information.  Then it compares against `values`, which at this point is a tzaware `DatetimeIndex`, so now raises a `TypeError` for lack of tzawareness-compat.

I'll have to re-run the whole suite to be sure, but I think that xfailing this one test will get to green without piling on hacks in internals.  I'll circle back to try to de-hack that after wrapping up the current set of PRs.
> I like this plan, I'm excited to be a part of it.

not sure if you remember [what happens next](https://www.youtube.com/watch?v=d-sALU_hveA)
Fortunately we all know what to do when asked if we’re gods.
No longer interpreting a string in the timezone of the index it is being compared to (but raising an error), seems a big compatibility break to me.
@jorisvandenbossche you realize this makes you the staypuft of this conversation, right?
Does this need discussion on eg the mailing list?
Thoughts on this? If we knock this down I can look at replace_list.
Just pushed with edits per suggestions along with a whatsnew note.
ok I pushed a merge of master to this. ping on green.
Travis timeout, appveyor `___ TestDataFrameGroupByPlots.test_series_groupby_plotting_nominally_works ____`
Sorry for not having further participated in the discussion, but repeating from my comment above: no longer interpreting a string in the timezone of the index it is being compared to (but raising an error), seems a big compatibility break to me. 

And I think we certainly should not regard it is as just a bug fix (eg in the whatsnew notes).


Ping
did u see joris comment
It is a bit easy to say for me as I am not doing the leg-work here, but I think it would be good if somebody made kind of an overview of the different related issues (this one, https://github.com/pandas-dev/pandas/issues/18435, https://github.com/pandas-dev/pandas/pull/17920, possibly others) related to timestamp comparisons: what is the current behaviour for the different cases (comparisons vs slicing, strings vs timestamp objects, scalars vs datetimeindex/series), where are the inconsistencies, what are the different options to solve this, ...

As I have the feeling that https://github.com/pandas-dev/pandas/pull/17920 is going in a different direction as this one (but difficult to see the full picture), and it would not be good to introduce more inconsistencies

Eg I think it would be OK to behave differently for comparing with strings vs timestamps: for strings interpret it in timezone it is being compared to, for timestamps raise if comparing non-aware with aware. But that is AFAIK not what this PR is doing. 
> good if somebody made kind of an overview of the different related issues

That was the idea behind #18435.  Feel free to edit if that if the OP doesn't provide enough overview for your tastes.

> And I think we certainly should not regard it is as just a bug fix (eg in the whatsnew notes).

How to treat it in the whatsnew notes is above my pay grade.  But over the course of this PR's history I've become increasingly convinced that the current comparison behavior is Just Plain Wrong and should be treated like a bug.

The three options on hand are 1) this PR which makes `DatetimeIndex` comparisons behave like all other datetime-like comparisons, 2) edit the spec to explicitly make string comparisons a special case, 3) do nothing.  I'm going to assume away 3 and argue against 2.

AFAICT the main objection to 1) is that in conjunction with #17920 it breaks the equivalence between `ser.loc[lower:upper]` and `ser[(ser.index >= lower) & (ser.index <= upper)]`.  But consider what other equivalences are broken by 2:

- We can no longer count on `DatetimeIndex` comparisons to be transitive.  `a >= b` and `b >= c` no longer implies `a >= c`.
- Comparison-broadcasting becomes inconsistent.  `(index >= bound)[n]` no longer equals `index[n] >= bound` (unless we then go mess with `Timestamp`...)
- Comparison-boxing becomes inconsistent.  `index >= bound` no longer necessarily equals `index >= Timestamp(bound)`.
- Box-conversion becomes inconsistent. `index >= bound` no longer necessarily equals `index.astype(object) >= bound`

... and most of all, I am not remotely confident that this list is complete.  How many places across the code-base do comparisons with `DatetimeIndex` objects in one place or another?  I have no idea.

A tz-aware `DatetimeIndex` is not easy to get by accident.  If a user has one, they have made a decision that timezones matter.

-------
Any of the available options introduces an inconsistency _somewhere_.  AFAICT Option1 breaks a convenience equivalency, will do so loudly, and as a result will not snowball into other inconsistencies.

Special-casing string comparisons generates a whole mess of other potential (often silent) problems that can be avoided by enforcing behavior that is already canonical.
> That was the idea behind #18435. Feel free to edit if that if the OP doesn't provide enough overview for your tastes.

Ah, yes, forgot there was already quite some discussion there. Can you put your last comment there as well? Then I will answer there
Just pushed a commit that lets tzawareness compat slide for strings, enforces it for everything else.