BUG: Timestamp.replace chaining not compat with datetime.replace #15683
State: closed

 - [x] closes #15683 
 - [x] tests added / passed
 - [x] passes ``git diff upstream/master --name-only -- '*.py' | flake8 --diff``
 - [x] whatsnew entry


Labels:
name: Bug
name: Datetime
description: Datetime data dtype
name: Compat
description: pandas objects compatability with Numpy or Python functions

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=h1) Report
> Merging [#16110](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/844013b20c2a6230b1baeec1d5442e26b7930656?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master   #16110      +/-   ##
==========================================
+ Coverage   90.84%   90.86%   +0.01%     
==========================================
  Files         159      159              
  Lines       50771    50771              
==========================================
+ Hits        46122    46132      +10     
+ Misses       4649     4639      -10
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.64% <ø> (+0.01%)` | :arrow_up: |
| #single | `40.34% <ø> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `90.68% <0%> (-0.35%)` | :arrow_down: |
| [pandas/core/indexes/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZXMucHk=) | `95.33% <0%> (-0.1%)` | :arrow_down: |
| [pandas/plotting/\_converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL3Bsb3R0aW5nL19jb252ZXJ0ZXIucHk=) | `65.35% <0%> (+1.81%)` | :arrow_up: |

------

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

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

```diff
@@            Coverage Diff             @@
##           master   #16110      +/-   ##
==========================================
- Coverage   91.04%   91.01%   -0.03%     
==========================================
  Files         162      162              
  Lines       49555    49567      +12     
==========================================
- Hits        45115    45114       -1     
- Misses       4440     4453      +13
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `88.79% <ø> (-0.01%)` | :arrow_down: |
| #single | `40.24% <ø> (-0.07%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `25% <0%> (-58.34%)` | :arrow_down: |
| [pandas/core/dtypes/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbmNhdC5weQ==) | `98.07% <0%> (-0.21%)` | :arrow_down: |
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.07% <0%> (-0.14%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.72% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `94.21% <0%> (-0.1%)` | :arrow_down: |
| [pandas/core/indexes/category.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9jYXRlZ29yeS5weQ==) | `98.52% <0%> (-0.01%)` | :arrow_down: |
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.15% <0%> (ø)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `92.03% <0%> (+0.04%)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `95.93% <0%> (+0.05%)` | :arrow_up: |
| [pandas/compat/numpy/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree#diff-cGFuZGFzL2NvbXBhdC9udW1weS9fX2luaXRfXy5weQ==) | `94.11% <0%> (+0.17%)` | :arrow_up: |
| ... and [1 more](https://codecov.io/gh/pandas-dev/pandas/pull/16110?src=pr&el=tree-more) | |

------

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

Looks like the tests are failing b/c I used `datetime.datetime.timestamp()` which is new in Py3.3. 
@jreback It's not only a compatibility issue. `Timestamp.replace()` computes wrong results under certain circumstances that would still be wrong if there was no `datetime.replace()` to be compatible with.
@sscherfke this is like adjusting, then readjusting things again. Start with the code I put up before.
@jreback The proposed `tz_localize_to_utc` approach leads to [this error](https://github.com/pandas-dev/pandas/issues/15683#issuecomment-295653478). 

Since all my attempts to fix localizing the timestamp failed, I just thought "why not just calculate a new value for the given time-struct and tzinfo?".  I know that this code is not the very best solution but this was the best that I could come up with based on the documentation available:
```python
# What I actually want to to is:
#
#     value = value_for(dts, tzinfo)
#
# The easiest thing I found that does this is creating a tsobject from a datetime() and taking its "value":
ts_input = datetime(dts.year, dts.month, dts.day, dts.hour, dts.min, dts.sec, dts.us, tzinfo=_tzinfo)
ts = convert_to_tsobject(ts_input, _tzinfo, None, 0, 0)
# Since datetime() has no nanoseconds, add them manually:
value = ts.value + (dts.ps // 1000)
```
The good thing of this approach is that we don't calculate any wrong offsets and that there also won't be any false "ambiguous time" errors.

I'd gladly improve the above code with anything that *correctly* calculates `value = value_for(dts, tzinfo)`
How do we proceed now?

My solution is not optional but unfortunately, I have no idea how to improve it.

Nonetheless, it fixes an obvious bug and the tests are passing now, so I’d be really happy if this gets merged.
@sscherfke can you run the asv's and post any discrepancies.
@jreback I ran `asv continuous -f 1.1 -b replace origin/master HEAD` (the replace bench. seems to be the only one affected by this patch) and it said everything is okay.
> @jreback I ran asv continuous -f 1.1 -b replace origin/master HEAD (the replace bench. seems to be the only one affected by this patch) and it said everything is okay.

pls run ALL the benchmarks. The issue is that this could affect a LOT of things (prob not, but checking). ``.replace`` is called by the dateutil code itself.
I've run the complete avs suite three times now, but I got strange/random looking results. E.g.:
"Bad" results from the first run:
```
+     1.08±0.01ms      1.62±0.01ms     1.50  io_bench.read_parse_dates_iso8601.time_read_parse_dates_iso8601
+      5.42±0.2ms       7.78±0.2ms     1.44  io_bench.read_csv_infer_datetime_format_custom.time_read_csv_infer_datetime_format_custom
+     9.89±0.02ms       13.9±0.1ms     1.41  inference.to_numeric_downcast.time_downcast('int32', 'signed')
+      25.0±0.5ms       30.2±0.8ms     1.21  groupby.groupby_agg_builtins.time_groupby_agg_builtins2
+        1.50±0ms       1.82±0.2ms     1.21  io_bench.read_csv_infer_datetime_format_ymd.time_read_csv_infer_datetime_format_ymd
+     7.72±0.01ms       9.30±0.7ms     1.20  io_bench.frame_to_csv_date_formatting.time_frame_to_csv_date_formatting
+     1.57±0.03ms       1.85±0.1ms     1.18  inference.to_numeric_downcast.time_downcast('datetime64', 'float')
+       419±0.5ms         485±30ms     1.16  groupby.Groups.time_groupby_groups('object_large')
+       157±0.9ms          179±2ms     1.14  frame_methods.Dropna.time_dropna_axis1_all_mixed_dtypes
+       150±0.4ms         172±10ms     1.14  io_bench.frame_to_csv_mixed.time_frame_to_csv_mixed
+     2.66±0.03ms       2.99±0.1ms     1.12  binary_ops.Ops.time_frame_add(True, 1)
+      53.7±0.2μs       59.4±0.1μs     1.11  timeseries.SemiMonthOffset.time_end_incr_n
+     2.61±0.03ms      2.88±0.04ms     1.10  binary_ops.Ops.time_frame_mult(True, 1)
+      53.3±0.1μs       58.8±0.1μs     1.10  timeseries.SemiMonthOffset.time_begin_incr_n
```

And here are the bad results of the third run (I failed to copy the results of the second run before they vanished beyond my bash history):
```
+     1.38±0.01ms      1.82±0.09ms     1.32  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('Micro', 1)
+      17.1±0.2ms         20.9±1ms     1.22  frame_methods.frame_fancy_lookup.time_frame_fancy_lookup_all
+     1.39±0.01ms      1.67±0.07ms     1.20  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('Easter', 2)
+      76.8±0.4ms         91.7±4ms     1.19  gil.nogil_rolling_algos_fast.time_nogil_rolling_skew
+     1.34±0.01ms       1.59±0.1ms     1.19  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('Micro', 2)
+          15.2ms           18.0ms     1.18  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('FY5253Quarter_1', 2)
+     1.34±0.02ms      1.58±0.07ms     1.18  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('Milli', 2)
+      78.2±0.4ms         91.9±1ms     1.17  gil.nogil_rolling_algos_fast.time_nogil_rolling_std
+      69.3±0.6ms       81.0±0.6ms     1.17  gil.nogil_rolling_algos_fast.time_nogil_rolling_var
+      9.16±0.1ms       10.6±0.3ms     1.16  algorithms.Algorithms.time_add_overflow_second_arg_nan
+      8.66±0.1ms       9.99±0.2ms     1.15  frame_methods.frame_mask_bools.time_frame_mask_floats
+         131±2ms          150±2ms     1.15  frame_methods.series_string_vector_slice.time_series_string_vector_slice
+        1.01±0ms      1.16±0.02ms     1.15  indexing.DataFrameIndexing.time_loc_dups
+     7.27±0.05ms       8.28±0.4ms     1.14  groupby.groupby_size.time_groupby_dt_size
+     9.98±0.09ms       11.4±0.3ms     1.14  inference.to_numeric_downcast.time_downcast('int32', 'integer')
+      76.4±0.5ms       86.7±0.5ms     1.14  gil.nogil_rolling_algos_fast.time_nogil_rolling_min
+      12.3±0.1ms       13.8±0.6ms     1.13  io_sql.ReadSQLTypes.time_datetime_read_and_parse_sqlalchemy
+         108±1ms          122±2ms     1.12  frame_methods.frame_duplicated.time_frame_duplicated
+          9.76ms           10.9ms     1.12  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('FY5253Quarter_1', 1)
+     1.45±0.01ms      1.62±0.04ms     1.12  frame_methods.frame_assign_timeseries_index.time_frame_assign_timeseries_index
+      38.7±0.2ms       43.2±0.9ms     1.11  gil.NoGilGroupby.time_min_2
+     1.96±0.02ms      2.18±0.01ms     1.11  frame_methods.frame_fancy_lookup.time_frame_fancy_lookup
+           311ms            346ms     1.11  gil.nogil_read_csv.time_read_csv
+     1.53±0.01ms       1.70±0.1ms     1.11  frame_ctor.FrameConstructorDTIndexFromOffsets.time_frame_ctor('BQuarterEnd', 1)
+      50.9±0.3μs       56.3±0.2μs     1.11  timeseries.SemiMonthOffset.time_end_incr
+        68.1±1ms       75.3±0.3ms     1.11  gil.nogil_rolling_algos_fast.time_nogil_rolling_mean
+     13.0±0.06ms      14.3±0.04ms     1.10  parser_vb.read_csv2.time_comment
+     1.95±0.01ms      2.15±0.02ms     1.10  frame_methods.Interpolate.time_interpolate_some_good_infer
```
As you can see, there's very little overlap between the two datasets (only `time_downcast('int32', 'signed')` is in both datasets), so I don't know how much I would trust the results.

Anyway, I still think that *correct* code is more important than a few percent of performance.
@sscherfke there is often quite some variation in the timings from one build to another (which makes it sometimes difficult to interpret a single run). So ratio's around 1.3 can most of the time be considered as 'normal', unless they would show up consistently. 

In the first run the first two are related to datetimes, so maybe you could repeat those to see whether the slowdown is consistent.

> Anyway, I still think that correct code is more important than a few percent of performance.

Yep, but running the asv is also a way to at least have an idea (and to indicate where a possible problem could be), and if there is a performance problem to look into ways to improve that keeping the correctness.
I’m really getting tired of this. I literally spent days trying to understand Pandas and to find a solution that works correctly. I had very little constructive help in the process.

When I finally created he PR, I hoped for a "Thanks for your efforts. *merged*" but all I’m getting is pointless nit-picking on my solution.

If that's the way you treat new contributors, good luck for the future. Do whatever you like with this PR. I'm out.
@sscherfke I am very sorry you feel that way. And thanks for your feedback on the process. As it is certainly important to use how new (or old) contributors experience this.

But, you also have to understand that a PR will almost *never* be just "merged, thanks". On the one hand we want to keep / obtain a high quality code base where new changes meet certain requirements, do not introduce other bugs or have large performance improvements (and not saying your PR would not meet that! just pointing out in general). And also we want to keep a certain consistency in the codebase (so just merging all PRs as we get them, would result in too much variety). 
What I am trying to say is that PRs will almost always get comments and reviews, often an iterative process (which can be tiring due the non-direct lagged interaction through github).

But at the same time, we certainly have to be aware of that such a process can become nitpicky, and try to avoid that!

For this PR, I hope you would like to reconsider trying to finish it, as this is a regression that would be nice to see fixed.
In order to do that, let my ask following questions:

- @jreback Are you OK with the *general* approach of this PR?
- For the *specific* question of @sscherfke about the construction of a Timestamp in cython form all its parts (see https://github.com/pandas-dev/pandas/pull/16110/files#r114604812), is there such a function? (@jreback?) Or is the approach taken OK? (I didn't directly find one)
- @sscherfke Would you like to rerun specifically the two benchmarks that showed a potential slowdown to see whether this is systematic? (see my question above https://github.com/pandas-dev/pandas/pull/16110#issuecomment-302015996)



> But, you also have to understand that a PR will almost never be just "merged, thanks".

I know. I exaggerated a bit to make my point. I’ve contributing to and maintining my own OSS projects since many years now.

The main drawback of my patch is the creation of a `datetime` instance but, as I said multiple times before, I don't know any low-level pandas function that does exactly what I want. The previous implementations/souggestions by @jreback calculated wrong results.
the general approach is ok as long as there are not perf regressions. This is a very very narrow code change, so yes certainly correctness is first, BUT if this causes perf issues with the vast majority of code would need another soln. I don't think perf is actually a problem, but needs some checking.

``.replace`` is actually implicity called a bunch by ``pytz`` so this is non-trival and prob not covered by our asv's.
While looking in the PR / this issue, I wondered the following:

```
In [97]: pd.Timestamp("2016-03-27 03:00", tz='CET')
Out[97]: Timestamp('2016-03-27 03:00:00+0200', tz='CET')

In [98]: pd.Timestamp("2016-03-27 03:00", tz='CET').replace(hour=1)
Out[98]: Timestamp('2016-03-27 01:00:00+0200', tz='CET')

In [99]: dt = datetime.datetime(2016, 3, 27, 3)

In [101]: tzinfo = pytz.timezone('CET').localize(dt, is_dst=False).tzinfo

In [102]: datetime.datetime(2013, 10, 27, 3, 0, tzinfo=tzinfo)
Out[102]: datetime.datetime(2013, 10, 27, 3, 0, tzinfo=<DstTzInfo 'CET' CEST+2:00:00 DST>)

In [103]: datetime.datetime(2013, 10, 27, 3, 0, tzinfo=tzinfo).replace(hour=1)
Out[103]: datetime.datetime(2013, 10, 27, 1, 0, tzinfo=<DstTzInfo 'CET' CEST+2:00:00 DST>)
```

So the `replace` is actually producing invalid times? (as due to the hour change, the timezone should be changed from +2 to +1). Which is both for `datetime.datetime` and `pd.Timestamp`, so not necessarily related to this PR. But, it is the reason I think that the (simpler) patch of de-localizing to naive in the beginning and re-localizing at the end during the `replace` method does not work, as errors can get raised in the re-localization when you get invalid dates.
Any chance that this gets eventually merged? None of the core devs have come up with a better or more performant solution and the bug is still a bug …
I haven't gone through the whole discussion, but IIRC we needed a new ASV benchmark in addition to the existing ones. I added a simple one to https://github.com/TomAugspurger/pandas/tree/sscherfke-master

```python
class TimeTimestamp(object):

    goal_time = 0.5

    def setup(self):
        self.ts = pd.Timestamp("2016-03-28")  # after DST
        self.tzinfo = pytz.timezone("CET")

    def time_timestamp_replace(self):
        self.ts.replace(tzinfo=self.tzinfo).replace(tzinfo=None)
```

Output

```
       before           after         ratio
     [15db50bb]       [ab7ea875]
+      10.8±0.2μs       18.3±0.8μs     1.70  timestamp.TimeTimestamp.time_timestamp_replace

```

So there's some work to be done there. I can take a look after Scipy next week.
> For the specific question of @sscherfke about the construction of a Timestamp in cython form all its parts (see https://github.com/pandas-dev/pandas/pull/16110/files#r114604812), is there such a function? (@jreback?)

This question is still unanswered.
if you can rebase and move docs to 0.21.0
I think this problem (https://github.com/pandas-dev/pandas/pull/16110#issuecomment-314029898) is still a problem for updating this? (unless the current one using stlib `datetime.datetime` is not a performance bottleneck)
Yes, it would still be a problem but none of the core devs has come up with a good solution yet. I don’t know enough about Pandas to improve my solution.
Maybe you just gotta descide whether in this particular case you want good performance *or* correct results (at least for now).

If you should decide that correct results are more important, I’d glady update my PR for the next release.
https://github.com/pandas-dev/pandas/pull/17356 is a clean version of this PR.