Support is_dst indicators in tz_localize
State: closed

The indicators are useful for assigning the correct offset when there are ambiguous transition times 
closes #7943.


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

Comments:
`is_dst` should accept a `boolean` _or_ and `ndarray-like` array (equal length to the series), check this.

It does, that's why it's ndarray rather than a typed ndarray.  I will add a test.

so

```
dr = date_range(datetime(2011, 11, 6, 0), periods=5,
                        freq=datetools.Hour())
```

these should both work (accept a list-like, use `com.is_list_like` to detect), then convert to a ndarray (you may need to do in a try-except in case its non-convertible)

```
dr.tz_localize('US/Eastern',is_dst=True)
dr.tz_localize('US/Eastern',is_dst=[1,1,0,0,0])
```

also if `is_dst` is not None and `infer_dst` is True that should raise (ambiguous options)

I misunderstood.  Right now [1,1,0,0,0] or [True,True,False,False,False] are what are supported.  I'd almost rather have it be explicit.  If the times aren't ambiguous then `is_dst` doesn't really do anything.

`is_dst` and `infer_dst` does raise.  I didn't add a test, though.

hmm, ok then so passing a single boolean is meaning-less (then default it to `None` and make it an error if its not-list like then).

and make the doc-string clear (for both this and `infer_dst`)

are you sure that its not better to simply make this one option?

e.g.

`infer_dst=True/False/boolean-list-like` ?

I do default to None and if the length is not the same as the passed values it errors.  

I guess we could put it in infer, although one really isn't inferring at that point, but I don't know if it is worth the extra argument.

_really_ should change the arg to:

`is_dst=None|boolean-like-list|'infer'` (and just drop `infer_dst`)

How do we handle that from a backwards-compatibility standpoint?

easy, deprecate `infer_dst` on the public methods (we are going to completely eliminate on the low-level stuff), see https://github.com/pydata/pandas/blob/master/pandas/core/frame.py#L2575.

Then add the new kw. You could need to process the old keyword if it has a value passed and translate to the new kw. (do this at the very top-level of the functions). Then simply pass the new kw to the lower level stuff and do your thing.

K, I do like this better.  I will update.

Updated using the `ambiguous` argument.  I'll update the docs once the code looks good.

some remarks:
- Is this API break necessary? As far as I understand, you want to add new functionality that does not really fit under the existing keyword `infer_dst`? But you could also add a second keyword to not break the API?
- The way you implemented it now seems backwards incompatible (not just deprecating), as True/False (the previous options for `infer_dst`) are not valid options anymore? So it's not only changing the keyword name, but also the values. The values True/False should at least also be deprecated first, not just removed I think. 
- About the new keyword name `ambiguous`, this name seems a bit 'ambiguous' to me .. :-) Just joking, but to say that something like `infer_dst` and `is_dst` is much more explicit that it is about the handling of DST ambiguities, so I personally like something like that more.

Furthermore, you said you still have to update the docs, but two ideas for that: I would provide some examples in the docstring to make the different options of the new kwarg more clear. And the DST handling can maybe also be promoted to it's own subsection within 'Time Zone Handling' in timeseries.rst.

-Two people both suggested the change and it seems to make sense.  The two keyword arguments refer to the same thing and are mutually exclusive so it makes sense to move it into one.
-The old method of True/False still works, it's just not documented as it doesn't make sense under the new construct.  Nevertheless, all old code will work and it's explicitly tested for.
-I think in the context of `tz_localize`, ambiguous makes sense.  Both `infer_dst` and `is_dst` are very specific to instances of how to treat ambiguous times.  I was originally going to go with `is_dst`, but @ischwabacher suggested the change and I went with it (see the issue for a more thorough description).  I like it for now and I spent enough time futzing with it already.

@rockg I agree 

I think this is a pretty speciality case but is useful for those who need it (and doesn't impact others)

is th NaT argument a string? (I think it should be for consistency but 'nat' is clear I think)

@ischwabacher ok with the args and concept?

Yes, it's a string.

Other than the doc note above, LGTM.

This is a change from how it works now.  True is translated into 'infer' (which I think is what you are intending by your comments).  Do we want that to be the default as it's a change as it currently raises an Exception?  I would almost rather leave it as an exception as users need to watch and understand how their data is being translated across transitions.  These nuances are easily missed and mishandled by users.

the default should not change (if you want to propose that, that's a separate issue).

so since `infer_dst=False` is currently the default, then `abmiguous=None` (or equivalent) should be the default. I agree that a user will have to explicity set this

Hmm... I would expect with this API that `True` be broadcast to mean effectively `is_dst=True`.  So you could do this:

``` python

In [1]: import pandas as pd

In [2]: pd.date_range('2013-11-3', periods=25, freq='H')
Out[2]: 
<class 'pandas.tseries.index.DatetimeIndex'>
[2013-11-03 00:00:00, ..., 2013-11-04 00:00:00]
Length: 25, Freq: H, Timezone: None

In [3]: _.tz_localize('America/Chicago', ambiguous=True)
Out[3]:
<class 'pandas.tseries.index.DatetimeIndex'>
[2013-11-03 00:00:00-05:00, ..., 2013-11-04 00:00:00-06:00]
Length: 25, Freq: None, Timezone: America/Chicago

In [4]: _.hour
Out[4]: 
array([ 0,  1,  2,  3,  4,  5,  6,  7,  8,  9, 10, 11, 12, 13, 14, 15, 16,
       17, 18, 19, 20, 21, 22, 23,  0])

In [5]: np.diff(Out[3].values) / pd.offsets.Hour(1).nanos
Out[5]: 
array([1, 2, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
       1], dtype='timedelta64[ns]')

```

We should probably have a kwarg for handling nonexistent times, too, since there are multiple options for them; we could take the offset before the discontinuity and yield a time after the discontinuity, or vice versa, or we could just return the time at the boundary, or we could return `NaT`, or we could raise.  Or, come to that, we could return a non-normalized time so that `date_range` could agree with incrementally adding to a time, but that seems horrible.

I didn't think about the fact that `raise` is the current default behavior; I agree that changing that is a separate decision.

Passing ambiguous in your example does nothing as there are no duplicate hours (you need to have duplicate 1:00, 1:00 for it to do anything).  True is just 'infer' (although it's merely there to support the old argument when 'infer_dst' was a bool).  Further, the bool-ndarray _only_ applies to the transition hours, not any times that tz_localize will handle appropriately otherwise.  The ambiguous logic only comes into effect when the times are ambiguous.

But the second time in the range _is_ ambiguous.  `'infer'` won't help because it's not duplicated (we don't _want_ it to be duplicated), but adding `ambiguous=True` does nothing for the other 24 times and only specifies that we should resolve the one ambiguous time to its first occurrence.  Then `t.hour` is constant between successive elements of `Out[3]`, which is an unintuitive behavior for most purposes but was the behavior I needed for the project that got me using pandas in the first place.  This is another reason why `is_dst` is misleading: most of the times aren't in DST, but we don't care because we don't want any _ambiguous_ times to resolve in standard time.

If we want `True` to mean `'infer'`, maybe we should just deprecate `infer_dst` rather than removing it, and have it set `ambiguous='infer'`.

`infer_dst` is deprecated and in the code `ambiguous=True` is set to 'infer' (check tslib.pyx) in my commit.  

'infer' or True will not work in your example.  If you pass in the bool-array you will get the behavior you expect.  'infer' only works when there are two ambiguous times, but less or more than that you need to explicitly tell it through the array.

@rockg What @ischwabacher means (I think), is to only deprecate `infer_dst` and not to _replace_ it by `ambiguous`. So for now keep both as seperate options, but deprecate `infer_dst` pointing users of `infer_dst=True` to `ambiguous='infer'`. 
This way `True/False` can be used for other things (eg the behaviour @ischwabacher describes which is in line with the behaviour of `is_dst` I think?) because it does not have to be the same as `'infer'`.

I used the `deprecate_kwarg` decorate which replaces `infer_dst` with `ambiguous` behind the scenes.  So it's only deprecated.  I think we should be able to add a message to the deprecate_kwarg decorator to the effect of "the meaning of True may change in future versions, change to `ambiguous='infer'`".  I'm not in favor of adding infer_dst back to the function calls.

Exactly what @jorisvandenbossche said, only I missed that `infer_dst` is still there and deprecated.  Derf.  It might be nice for `deprecate_kwarg` to be able to transform its inputs-- I'll add a PR for that.

Yes, for the behaviour I described above, you cannot use `deprecate_kwarg`. As this only works for merely a renaming of a kwarg, not for changing behaviour.

@rockg you prob need to do a 'manual' deprecation here https://github.com/pydata/pandas/blob/master/pandas/core/frame.py#L1279 for an example to maybe have more control (ironically that is an older example which _could_ use the decorator)

@rockg About adding back `infer_dst` to the function call (but still deprecated as it is now), the question is more about the API for `ambiguous`: _if_ we want to use `True/False` for something else than 'infer'/'raise', then we should just do that (that is the possibility we have now because we are changing the keyword) and don't let us stop from doing that because it is a little bit more code to deprecate `infer_dst`. 
(BTW, you can also work with `**kwarg` if you really don't want it to show up in the function call)

@rockg and then some answers to the other points:
- personally I think that having two mutually exclusive keywords is not really a problem (you can provide nice error message for that), certainly if it would lead to a cleaner API instead of one overloaded keyword. But for this case, I don't have a strong opinion, so it's OK, discussion over.
- About (not) documenting the old True/False options: in case we keep it as it is now in the PR, I think we should choose to either deprecate those values (and thus provide a deprecation warning) or document it. As it is not a good idea to have some not described behaviour of a keyword I think.
- About the name of the keyword argument (`ambiguous` vs `is_dst`/ `handle_dst` ...): let me explain why I said it: I think for the name of a kwarg, we should ask ourselves: _"which question does it answer?"_ And so I was thinking: the `True/False` in the bool array is the answer on "Is this timestamp a DST?" And not on "Is this timestamp ambiguous". And in this reasoning I did find `is_dst` much more logical.  
  But I now understand that it is not as simple as that .. :-) (I have to admit, I am not really familiar with this, but I think in that way it is just a good exercise to see if the docs are understandable for somebody like me!)

And +1 on renaming the default from None to the more explicit `'raise'`

I think we are all set with the latest commit.  I added back `infer_dst` and added deprecation messages to the three places it could be used.  Docs/doc strings were updated and default argument is now 'raise'. 

1b60d14 has the merge conflict but I can't get rid of it.  I went to rebase but only 1b60d14 is there, c52204a is not (however it shows up in the rebase except below...not modifiable but saying rebasing that commit).  Any thoughts?

```
pick 70a17da COMPAT: capture chain indexing even on single-dtyped
pick 4a23a84 ENH/CLN: add HistPlot class inheriting MPLPlot
pick 1b60d14 New "ambiguous" argument in tz_localize to enhance support for ways of resolving transition times ('infer', dot indicators, 'NaT', and 'raise')

# Rebase fa63e76..c52204a onto fa63e76
```

Also, another git question.  Usually I do git fetch upstream, git merge upstream/master, git reset --hard.  After the second step when running git status there are a bunch of "Changes to be committed", presumably all the changes that were committed on master since my last fetch.  Usually git reset --hard removes all of these can I can continue on.  However, when there's a merge conflict I need to resolve the conflict, add the file and then commit which would then commit all of these changes.  Is that okay or should I be doing this some other way?