handle DST appropriately in Timestamp.replace
State: closed

```
ts = pd.Timestamp('2017-12-03 16:03:24', tz='US/Eastern')
# Timestamp('2017-12-03 16:03:24-0500', tz='US/Eastern')

ts2 = ts.replace(month=6)           # <-- across DST boundary
# master --> Timestamp('2017-06-03 16:03:24-0500', tz='US/Eastern')
# PR     --> Timestamp('2017-06-03 16:03:24-0400', tz='US/Eastern')

ts3 = ts2.tzinfo.normalize(ts2)
# master --> Timestamp('2017-06-03 18:03:24-0400', tz='US/Eastern')
# PR     --> Timestamp('2017-06-03 16:03:24-0400', tz='US/Eastern')

assert ts3 == ts2
assert ts3.hour == ts.hour
```

Both assertions fail under master, pass under this PR.

The whatsnew entry is not super-informative.  Any ideas for a one-sentence description of this fix?

- [x] closes #18319

xref #7825 (I think)
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

  

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

Comments:
> closes #18319, closes #7825 (I think)

Any reason why you're not sure about #7825.  If you can run the example code without breaking, that would be considered sufficient to close.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18618?src=pr&el=h1) Report
> Merging [#18618](https://codecov.io/gh/pandas-dev/pandas/pull/18618?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/6e56195fcb021e7c53301722804087ef2a83bc39?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #18618      +/-   ##
==========================================
- Coverage   91.46%   91.45%   -0.02%     
==========================================
  Files         157      157              
  Lines       51449    51449              
==========================================
- Hits        47060    47051       -9     
- Misses       4389     4398       +9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.32% <ø> (ø)` | :arrow_up: |
| #single | `40.6% <ø> (-0.11%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18618?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.81% <0%> (-0.1%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/18618?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/18618?src=pr&el=footer). Last update [6e56195...cfb760c](https://codecov.io/gh/pandas-dev/pandas/pull/18618?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/18618?src=pr&el=h1) Report
> Merging [#18618](https://codecov.io/gh/pandas-dev/pandas/pull/18618?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/2dac79303297ea8cab9e11596159bff6f5e4738b?src=pr&el=desc) will **decrease** coverage by `0.05%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #18618      +/-   ##
==========================================
- Coverage   91.57%   91.51%   -0.06%     
==========================================
  Files         150      148       -2     
  Lines       48937    48804     -133     
==========================================
- Hits        44815    44664     -151     
- Misses       4122     4140      +18
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.89% <ø> (-0.05%)` | :arrow_down: |
| #single | `41.63% <ø> (-0.13%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/18618?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.22% <0%> (-1.74%)` | :arrow_down: |
| [pandas/core/computation/expressions.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tcHV0YXRpb24vZXhwcmVzc2lvbnMucHk=) | `93.27% <0%> (-1.69%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `84.41% <0%> (-0.54%)` | :arrow_down: |
| [pandas/io/pytables.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3B5dGFibGVzLnB5) | `92.45% <0%> (-0.4%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.17% <0%> (-0.28%)` | :arrow_down: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `92.62% <0%> (-0.19%)` | :arrow_down: |
| [pandas/tseries/frequencies.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvZnJlcXVlbmNpZXMucHk=) | `93.9% <0%> (-0.15%)` | :arrow_down: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `95.9% <0%> (-0.05%)` | :arrow_down: |
| [pandas/core/panel.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcGFuZWwucHk=) | `96.83% <0%> (-0.02%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.45% <0%> (ø)` | :arrow_up: |
| ... and [7 more](https://codecov.io/gh/pandas-dev/pandas/pull/18618/diff?src=pr&el=tree-more) | |

------

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

lots of tests in #7825, can you add tests / narrow down what is and is not fixed.
> Any reason why you're not sure about #7825. If you can run the example code without breaking, that would be considered sufficient to close.

@gfyoung as jreback mentioned, that issue covered a lot of ground.  The crash there can be ignored, not a pandas issue.  The OP there gave an example of Timestamp.replace that was incorrect, but did not give the expected output.
@ischwabacher can you have a look
needs a rebase. also pls add all of the examples from #7825 ; if something doesn't work, then you can open new issue for that part.
further there are a number of timezone issues that this (and/or other fixes) might have resolved. should investigate. anyone want to take this on? @gfyoung @chris-b1 ?
> needs a rebase.

Will do.

> also pls add all of the examples from #7825 ; if something doesn't work, then you can open new issue for that part.

#7825 is all over the place (I probably shouldn't have referenced it above).  The example in the OP I _think_ is handled, but the poster didn't specify the expected behavior.  I think the thing to do is keep that open and ask the OP there to verify if the new behavior closes that issue.

> I would like to include some tests on the original issue #7825 if they are not working xfail them.

I'll take another look, but based on the last two looks I concluded that this is not actionable until the author there provides the expected output for the example cases.

Simpler to treat #18319 as the "original issue". 
There's a follow-up to this waiting in the wings that fixes a couple of related bugs.
I think this needs at least two additional tests - 1. a test for when `datetime.replace` creates an ambiguous time and 2. a test for when `datetime.replace` creates an imaginary time.

I *think* that the way it's done right now, `pytz` and `dateutil` zones will do different things in these situations.
> I think this needs at least two additional tests - 1. a test for when datetime.replace creates an ambiguous time and 2. a test for when datetime.replace creates an imaginary time.

Good idea, will do.
I bet this will fix #18785 as well.
I added an example for #18785 
> I think this needs at least two additional tests - 1. a test for when datetime.replace creates an ambiguous time and 2. a test for when datetime.replace creates an imaginary time.
>
> I think that the way it's done right now, pytz and dateutil zones will do different things in these situations.

I've added these tests and you're right, these do different things.  1AM on 2014-11-05 is ambiguous in US/Eastern:

```
>>> ts_dateutil = pd.Timestamp('2014-11-05 01:00:00', tz=dateutil.tz.gettz('US/Eastern'))
>>> ts_pytz = pd.Timestamp('2014-11-05 01:00:00', tz=pytz.timezone('US/Eastern'))
>>> ts_dateutil == ts_pytz
True
>>> res_dateutil = ts_dateutil.replace(day=2)
>>> res_dateutil = Timestamp('2014-11-02 01:00:00-0400', tz='dateutil//usr/share/zoneinfo/US/Eastern')
>>> res_pytz = ts_pytz.replace(day=2)
>>> res_pytz
Timestamp('2014-11-02 01:00:00-0500', tz='US/Eastern')
```

So the pytz version resolves to the later of the two possibilities while the dateutil version resolves to the earlier.  It isn't clear to me that there is an unambiguous "right" thing to do in this case, so as far as pandas is concerned just deferring to pytz/dateutil may be the way to go.  Thoughts?

As for imaginary times, both pytz and dateutil versions (both in the PR and in master) fail to raise when `ts.replace()` results in a non-existent time.  My preference would be to address this in a separate issue/PR.
> So the pytz version resolves to the later of the two possibilities while the dateutil version resolves to the earlier. It isn't clear to me that there is an unambiguous "right" thing to do in this case, so as far as pandas is concerned just deferring to pytz/dateutil may be the way to go. Thoughts?

There is indeed an unambiguous right way to do things. The Python documentation before 3.6 suggested that ambiguous times should always resolve to the standard time (the later time). As of Python 3.6, unfortunately this is inverted because the default value of `fold` is 0 and `fold=1` is interpreted as the *second* time. So dateutil is correct in this case, but you are welcome to defer this as "`pytz`'s problem".

> As for imaginary times, both pytz and dateutil versions (both in the PR and in master) fail to raise when `ts.replace()` results in a non-existent time. My preference would be to address this in a separate issue/PR.

No time zone hooks are called on `replace` or on any constructor. If you want to retain the semantic meaning of `replace`, then do nothing. If you want `replace` to always produce a valid date, it's necessary to choose a default behavior.

To me, it seems like this PR is mostly about making `pytz` act like `dateutil`, since `dateutil` implements the pythonic `tzinfo` interface and `pytz` uses its own interface. If you think of it this way, then you should basically just be supporting PEP 495 fold for ambiguous times (not difficult to backport when you're already wrapping `datetime` internally) and allowing `replace` to create imaginary times (though if you want to enforce real datetimes in a separate PR, or add some mechanism to normalize imaginary datetimes, that's reasonable).
did you cover @pganssle requests for tests?
  
> did you cover pganssle requests for test?

@pganssle correct me if I'm wrong but I think the consensus was that there are some lower-level changes that might avoid this class of problem longer-term, but it was OK to consider that out of scope for this PR.  (Possibly what you had in mind for #18595?).
@jreback While #18595 and this issue share the same root cause - namely that `pytz` uses a different mechanism for assigning time zones than what is expected in the `datetime` library - the solutions are different. #18595 is just about having a canonical time zone object for a given series, whereas this issue is about the fact that after every operation with a datetime with attached `pytz` zone, the zone offsets need to be re-normalized.

To put it another way, if you think of pytz as having a "parent" zone (the one you get by calling `pytz.timezone`) and "child" zones (the ones that are attached to the `datetime` by `.localize` and `.normalize`, #18595 is about having a standard way to retrieve the "parent" zone, while this PR is about retrieving the correct "child" zone for a given datetime.
This is a sort of "out there" idea, but there is one (possibly not particularly performant) thing you could do that would unify the interface between `pytz` and `dateutil` (and all other zone providers), and also solve #18595, which is to write a wrapper around `pytz` that just has the standard `tzinfo` interface. A rough sketch (eliding some of the details like how to handle ambiguous and imaginary times, and how to handle `str` and `repr`, etc):

```python
from datetime import tzinfo
from datetime import datetime, timedelta

import functools

import pytz

def _localize(f):
    @functools.wraps(f)
    def inner_func(self, dt):
        return f(self, self._zone.localize(dt.replace(tzinfo=None)))

    return inner_func

class pytz_zonewrapper(tzinfo):
    def __init__(self, pytz_zone):
        self._zone = pytz.timezone(str(pytz_zone))

    @_localize
    def utcoffset(self, dt):
        return dt.utcoffset()

    @_localize
    def tzname(self, dt):
        return dt.tzname()

    @_localize
    def dst(self, dt):
        return dt.dst()
```

I would say for now go with @jbrockmendel's approach, but I would think that it might be nice to remove all the pytz-specific code cluttering up time zone handling.

Another option might be to just internally start using `dateutil` zones. I don't think it's *guaranteed*, but from my testing `tz.gettz(str(pytz_zone))` will always give you the appropriate, valid `dateutil` zone. I believe that `pytz` is still considerably faster than `dateutil` in many areas, which I'm hoping to take care of at some point soon but haven't gotten around to yet.
  
thanks, I left #7825 open until we resolve whether this closes it.