API/BUG: Enforce "normalized" pytz timezones for DatetimeIndex
State: closed

closes #3746
closes #18595
closes #13238
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Addressing 3 birds with 2 stones here. Using @pganssle suggested implementation for a tz property and directly raising an error per #3746 (could depreciate and error in a future version as well, open to feedback on the prefered path of API change)

Additionally, solves the issue of resampling a DataFrame/Series with a DatetimeIndex that retained a local timezone instead of the "LMT" version.  

Labels:
name: Bug
name: Timezones
description: Timezone data dtype
name: Resample
description: resample method

Comments:
I'm not sure I understand the purpose of making it so you can't set `tz`.

It's also *not* certain that you will get `LMT` for any given zone (though it is likely for certain zones). In general I think [people should not use pytz at all](https://blog.ganssle.io/articles/2018/03/pytz-fastest-footgun.html), but I'm aware that that might be a pretty big API change. Still, I think any sort of datetime implementation should try to *hide* `pytz`'s implementation details, not expose them directly.
> I'm not sure I understand the purpose of making it so you can't set tz.

Timestamp and DTI are immutable objects, so setting after creation doesn't make sense.


> It's also not certain that you will get LMT for any given zone (though it is likely for certain zones). In general I think people should not use pytz at all, but I'm aware that that might be a pretty big API change. Still, I think any sort of datetime implementation should try to hide pytz's implementation details, not expose them directly.

this is generally true. The actual tz is an implementation detail, which we don't really care / want to expose to the user directly (of course we want them to be able to construct using there favorite tz library & ISO). Users mostly just care about the iso repr anyhow (US/Eastern). After this PR things will more closely follow this pattern.
@jreback Looking more closely, I think I was slightly confused because the comments and documentation are misleading. None of these are testing or ensuring that the tz is "LMT", they are just ensuring that it is a consistent `tzinfo` object. I think it is best to scrub all mentions of LMT from the comments and documentation.
@pganssle Yeah I was mainly using `LMT` as an alias (and consistent pytz basis) for "the first tzinfo object returned from `pytz.timezone(...)`" since I thought it would always be `LMT`. Since that may not be the case, agreed that I should remove mention of `LMT` to avoid confusion
@mroeschke I don't think "the first tzinfo object" is really the contract either. The point is that `tz` should represent the *canonical* time zone object, to the extent that there is one (any one of them can be chosen as a representative, so long as it's always the same one).

That said, exposing that information anywhere in the public-facing documentation is probably a bad idea, because it's very much a `pytz` concept, and specifically one of the weird ones that violates the `tzinfo` interface. I think it would be better if pandas started moving away from any overt sign that they use pytz and try to move away from using pytz-specific concepts.

I think a property alone is enough to hide the implementation detail that the time zone object may be different for different `datetime` objects within the `DateTimeIndex`.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20510?src=pr&el=h1) Report
> Merging [#20510](https://codecov.io/gh/pandas-dev/pandas/pull/20510?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/aa3fefc898b2f101f3cf83f90add69857926fcbe?src=pr&el=desc) will **decrease** coverage by `0.02%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #20510      +/-   ##
==========================================
- Coverage   91.84%   91.82%   -0.03%     
==========================================
  Files         153      153              
  Lines       49256    49257       +1     
==========================================
- Hits        45241    45230      -11     
- Misses       4015     4027      +12
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.21% <100%> (-0.03%)` | :arrow_down: |
| #single | `41.91% <63.63%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/20510?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/indexes/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/20510/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZWxpa2UucHk=) | `96.72% <100%> (ø)` | :arrow_up: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/20510/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.73% <100%> (ø)` | :arrow_up: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/20510/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.07% <0%> (-1.74%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/20510?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/20510?src=pr&el=footer). Last update [aa3fefc...67a29d5](https://codecov.io/gh/pandas-dev/pandas/pull/20510?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Addressed your comments @jreback. 
> I think a property alone is enough to hide the implementation detail that the time zone object may be different for different datetime objects within the DateTimeIndex.

@pganssle what do you mean exactly here?

@mroeschke great work!
@jorisvandenbossche Not sure what part you are confused about, but my intention was that `tz_standardize ` should be as private as possible, not part of the public interface. Having time zone accessed exclusively through a `tz` property should be sufficient to achieve this, and thus end users should never actually have to call `tz_standardize`.

Ideally, the `tz` property *itself* would just be implemented as a `cpdef` and `tz_standardize` would be a `cdef` not accessible by end users.
> Ideally, the tz property itself would just be implemented as a cpdef and tz_standardize would be a cdef not accessible by end users.

@pganssle ALL of this is explicitly private already.
@jreback I think I was at least partially fooled by the fact that it has documentation - it's not always clear to me what ends up getting included in the docs.

Is it "explicitly private" in the sense that it's actually impossible for users to access from the Python API, or in the sense that it's under `_libs`? If the latter, then Hyrum's law indicates that someone will start using it eventually. I'm pretty sure there's a way to implement this as truly private.

That said, if it's a pain don't bother. I wasn't planning on harping on it, just explaining my comment.
@pganssle if completely private as its under ``_libs``. we can and do change these things w/o warning. If someone uses it, then tough luck. We have enough issues with the public API. 

note that ALL functions really should have documentation. private ones as well as public ones. We have been making efforts on this recently. It is just useful for future readers.
Sure, but the same could be said for `dateutil.parser._timelex`, the undocumented private class not in `__all__`, but you were the one who persuaded me to add a deprecation warning to that. I'm suggesting that these things go much easier when the interpreter literally won't let you access the symbols (though this is not always possible).
> things go much easier when the interpreter literally won't let you access the symbols (though this is not always possible)

that's not possible in this case, as we need to use `tz_standardize` (internally) in a python file

> Not sure what part you are confused about, but my intention was that tz_standardize should be as private as possible, not part of the public interface. Having time zone accessed exclusively through a tz property should be sufficient to achieve this

Confusion solved :) My starting point was that only `.tz` was already the only public exposure.

That said,  `.tz` *is* public, and returns a pytz instance:

> I think it would be better if pandas started moving away from any overt sign that they use pytz and try to move away from using pytz-specific concepts.

I suppose this would only be solved by no longer returning a pytz instance? This could potentially break peoples codes that use a pytz-specific method of the `tz` object?
> that's not possible in this case, as we need to use tz_standardize (internally) in a python file

This is not quite true. In `tslibs` you can do this:

```python
cdef tz_standardize(tz):
    ...

cpdef get_tz(self):
    return tz_standardize(self._tz)
```

And then you can just do:

```python
class DateTimeIndex:
    tz = property(get_tz)
```

This is one way to do it (might actually be faster because presumably the `tz_standarize` call is inlined into `get_tz`). There are other mechanisms that prevent exposing the `tz_standardize` symbol, but I'm not terribly worried about it honestly.

> I suppose this would only be solved by no longer returning a pytz instance? This could potentially break peoples codes that use a pytz-specific method of the tz object?

It doesn't actually return a `pytz` instance, it returns whatever `tzinfo` object you attached. If you use a `dateutil` timezone, it should return a `dateutil` timezone. If you're going to move away from `pytz`, I would change it so that passing an IANA timezone string to `tz_localize` attaches a `dateutil` zone, not a `pytz` zone, but as you mention that would probably be a breaking change.

Deprecating that behavior would be tricky, though. The best option for deprecation I think would be to create some sort of mock object that wraps `pytz` time zones and raises `DeprecationWarning` whenever any of the `pytz`-specific methods are called.
@jorisvandenbossche address your comment and all green. 
I'll try to get those ASV sometime tomorrow; my asv setup still doesn't work and need to look into it. 
I wasn't able to resolve my asv issues, but here's a timeit in the meantime. There appears to be a noticeable slowdown. 

```
In [1]: import pandas as pd

In [2]: dti = pd.DatetimeIndex([pd.Timestamp('2010', tz='US/Pacific')])

# This branch
In [3]: %timeit dti.tz
2.21 µs ± 25.8 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

#Master
In [4]: %timeit dti.tz
52.8 ns ± 1.37 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
```
@mroeschke Have you tried using the mechanism I mentioned [in this comment](https://github.com/pandas-dev/pandas/pull/20510#issuecomment-377802224), using `tz_standardize` as a `cdef`? That may end up being faster because it saves on Python function call overhead, but I'm not sure how much faster.

That said, it seems unlikely that anyone is accessing the `tz` property in a tight loop. It's an operation that happens once per index. I suspect spending *too* much time optimizing it might be a waste of time.
Actually, just tried it, a lot of the "slow" part is converting to the "canonical" `pytz` zone. It's much faster if you `tz` refers to a `dateutil` time zone.

Using a `pytz` zone:
```python
# master branch
In [4]: %timeit dti.tz
69.8 ns ± 0.826 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

# cdef tz_standardize
#master
In [8]: %timeit dti.tz
69.5 ns ± 1.25 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)

In [3]: %timeit dti.tz
2.41 µs ± 11.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

# cpdef tz_standardize
In [6]: %timeit dti.tz
2.79 µs ± 113 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
```

And for a `dateutil` zone:
```python
# cdef tz_standardize
In [25]: %timeit dti.tz
478 ns ± 31.7 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

# cpdef tz_standardize
In [4]: %timeit dti.tz
691 ns ± 4.62 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)
```

Another 190 ns is explained by using a property at all (and thus using the descriptor protocol). When `tz` is set to `return self._tz`, we get:

```python
In [4]: %timeit dti.tz
190 ns ± 1.11 ns per loop (mean ± std. dev. of 7 runs, 10000000 loops each)
```

I think the remaining timing is just the time it takes to determine if something is "pytz-like". That said, there's not much you can do about that, particularly since most people will be using `pytz` until and unless `pandas` starts to move towards using `dateutil` zones.
Thanks for the investigation @pganssle! I am getting similar results for the `get_tz` property implementation as the one on this branch.

```
In [1]: import pandas as pd

In [2]:  dti = pd.DatetimeIndex([pd.Timestamp('2010', tz='US/Pacific')])

# tz = property(get_tz)
In [3]: %timeit dti.tz
2.43 µs ± 153 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
```

For a dateutil tz, this should just some overhead with a `hasattr` check, but you're probably correct that for pytz, a good amount of time is spent constructing the new pytz.

But overall I agree with you that I do not suspect many accessing `.tz` in a tight loop.
> For a dateutil tz, this should just some overhead with a hasattr check, but you're probably correct that for pytz, a good amount of time is spent constructing the new pytz.

It's more than just that, it's also the function call overhead. To the extent that `%timeit` is reliable, inlining the extra call to `tz_standardize` does provide a reasonable (looks like it could be about 30%) speedup when using `dateutil` zones.
so what u need to do here is use the cache_only decorator
since these are immutable once constructed they r good
will this PR still work if instead of calling ``tz_standarize`` on the ``.tz``, you instead set the ``._tz`` with the standardized value? this sidesteps the caching as its not longer needed then.


Good call @jreback. Additionally, standardizing `._tz` directly allows us to provide a better error message when someone attempts to set `.tz`.

All green 
very nice @mroeschke ! keep em coming!