API/ENH: tz_localize handling of nonexistent times: rename keyword + add shift option
State: closed

- [x] closes #8917
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Currently, users do not have any control over nonexistent datetime handling when `tz_localize`ing like they do ambiguous times. This adds a new keyword `nonexistent` to `tz_localize` so that users now can:

``'raise'``: Raise an error (default)
``'NaT'``: Replace nonexistent times with ``'NaT'``
``'shift'``: Shift nonexistent times forward to the closest existing time


Labels:
name: Enhancement
name: API Design
name: Timezones
description: Timezone data dtype

Comments:
Hello @mroeschke! Thanks for submitting the PR.

 - There are no PEP8 issues in the file [`pandas/core/arrays/datetimes.py`](https://github.com/mroeschke/pandas/blob/710014cefece3c5e67604def64ab14cbf69205f0/pandas/core/arrays/datetimes.py) !

 - There are no PEP8 issues in the file [`pandas/core/generic.py`](https://github.com/mroeschke/pandas/blob/710014cefece3c5e67604def64ab14cbf69205f0/pandas/core/generic.py) !

 - There are no PEP8 issues in the file [`pandas/tests/indexes/datetimes/test_timezones.py`](https://github.com/mroeschke/pandas/blob/710014cefece3c5e67604def64ab14cbf69205f0/pandas/tests/indexes/datetimes/test_timezones.py) !

 - There are no PEP8 issues in the file [`pandas/tests/scalar/timestamp/test_timezones.py`](https://github.com/mroeschke/pandas/blob/710014cefece3c5e67604def64ab14cbf69205f0/pandas/tests/scalar/timestamp/test_timezones.py) !

 - There are no PEP8 issues in the file [`pandas/tests/series/test_timezones.py`](https://github.com/mroeschke/pandas/blob/710014cefece3c5e67604def64ab14cbf69205f0/pandas/tests/series/test_timezones.py) !


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

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

```diff
@@            Coverage Diff             @@
##           master   #22644      +/-   ##
==========================================
+ Coverage   92.22%   92.22%   +<.01%     
==========================================
  Files         169      169              
  Lines       50911    50922      +11     
==========================================
+ Hits        46954    46965      +11     
  Misses       3957     3957
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.65% <94.11%> (ø)` | :arrow_up: |
| #single | `42.28% <23.52%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22644?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/datetimes.py](https://codecov.io/gh/pandas-dev/pandas/pull/22644/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2RhdGV0aW1lcy5weQ==) | `97.45% <100%> (+0.05%)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/22644/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.79% <83.33%> (-0.05%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/22644/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `86.73% <0%> (+0.09%)` | :arrow_up: |

------

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

@jreback re: overlap between `errors` and `nonexistent`

1) The original issue mentions https://github.com/pandas-dev/pandas/issues/8917#issuecomment-65082306 having the ability to control over ambiguous times vs nonexistent times independently.
1a) `nonexistent` and `ambiguous` can handle their own errors in this PR independently
2) This PR has the ability to `shift` the nonexistent time to a real time (like how `ambiguous` can take `True/False`

So I would propose that eventually we can depreciate `errors` and keep both `ambiguous` and `nonexistent`
> So I would propose that eventually we can depreciate errors and keep both ambiguous and nonexistent

can you do that in this PR?
@jreback went ahead and depreciated the `errors` parameter in `tz_localize`
Addressed your comments @jreback, created tests checking for a `FutureWarning` for `tz_localize(..., errors=...)` in DTI/Timestamp/dt accessor, and all green.
Didn't look in detail at the diff, but general question. You mention in the top post:

> Currently, users do not have any control over nonexistent datetime handling when tz_localizeing like they do ambiguous times. 

But doesn't the `errors` keyword handle non-existent times? Or what else does that keyword handle? So you already have the option to error / coerce (only not to shift, what you added)


Hmm good point @jorisvandenbossche. I think this was a complete oversight on my part. Thanks.

I had thought `errors` affected _any_ localization errors this entire time, but you're right, it appears to specifically target nonexistent times.

I guess the consolation this PR brings is:

1) IMO `nonexistent` is a better argument name than `errors` (demonstrated by my oversight)
2) Addition of the `shift` method (as you mentioned).
3) Brings nonexistent time handling to `Series.tz_localize` (akin to adding errors to that method)

I can only really hang my hat on 1, but I understand if its not worth the API churn. Thoughts?
i don’t think api churn is really much of an issue here

this is a pretty niche use of the kwarg ; when u need it it’s very important but most of the time u don’t care

rather have the correct handling with rationale named things
I agree `nonexistent` is a better name than `errors`, but I am not fully sure it is that much better to warrant a deprecation. This is indeed a rather niche use case, but if the parameter description clearly indicates "How to handle non-existent time errors: ...", I think that also might be sufficient.

The addition of shift and addition of the functionality to `Series.tz_localize` are good enhancements for sure. Since for `Series.tz_localize` it is a new keyword, there is no backward compatibility problem in that case of course ..

It just feels a bit stupid that we are deprecating a keyword that we only relatively recently added for the exact usecase in this PR (apart from adding the shift option of course)

> rather have the correct handling with rationale named things

The concern I raise is not about not having "correct handling" or so, apart from the additional features added here in this PR, it is a pure keyword-rename for the rest.
> It just feels a bit stupid that we are deprecating a keyword that we only relatively recently added for the exact usecase in this PR (apart from adding the shift option of course)

sure, but would have rather have the new, descriptive keyword in a year than not. I don't think the prior deprecation period is relevant in the long term, though sure can produce churn. 

I don't see a reason not to proceed in this relatively niche case here. If it were a major impacting change (e.g. ``skiprows`` -> ``skip_rows``) or some such, then sure maybe it is more impactful, though that shoulnd't stop a good deprecation.
Addressed your comments @jreback 
Thanks @jorisvandenbossche. Addressed your comments. 
lgtm. @jorisvandenbossche merged when satisified.
lgtm @jorisvandenbossche any more comments?
Thanks @jorisvandenbossche. Added those blank lines for rendering. 
Thanks for catching that typo @jorisvandenbossche 
one more rebase and I think ok to go
thanks @mroeschke