ENH: Ability to tz localize when index is implicility in tz
State: closed

Fix to issue #4230 which allows to localize an index which is
implicitly in a tz (e.g., reading from a file) by passing imply_dst to
tz_localize.


Labels:

Comments:
@nehalecky - any comments on this?

To me, `imply_dst` is confusing as a term - it's not clear to me this would do, particularly because it looks like the equivalent `pytz` keyword accepts either `True` or `False`. I think you might mean that the dst is implied from the data, in which case `infer_dst` would make more sense as the keyword argument.

 It looks like pytz calls the marker for this [`is_dst`](http://pytz.sourceforge.net/#tzinfo-api):

> The `is_dst` parameter is ignored for most timestamps. It is only used during DST transition ambiguous periods to resulve that ambiguity.

If you have some logic that somehow decides whether or not to infer dst (which I think is what you're doing), you could call it `infer_dst` and specify how you decide whether it should be `is_dst=True` or `is_dst=False`. It looks like you're inferring it based upon looking for ordering (i.e., occurring every hour or something) - if that's the case, it would be useful to add a minimal example to the docs that shows how pandas is or is not able to infer timezone. (i.e., something around here - http://pandas.pydata.org/pandas-docs/stable/timeseries.html#time-zone-handling )

If `imply_dst` means that you do the equivalent of parsing every datetime as if you had passed `is_dst=True` to the pytz constructor, then it would make more sense to allow the value to be either `None`, `True` (or truthy) or `False` and then have it parse with the equivalent of `is_dst=False`, `is_dst=True` passed to the pytz constructor.  Probably would also make sense to change the name to `is_dst` and use something like the quoted doc above to explain it.

On the exception I was following the code that was already there, however it appears from the pytz source that it just subclasses exception so I will enhance the message.

"infer" is the right verb here.  I will make those changes.  I was planning on adding the docs, both the api and tz handling sections.  Also, it seems like the api docs are lacking mention of DatetimeIndex.  Is that intentional or just waiting for somebody to add it?

On your comment above about adding a comment on the line, was there something in particular you want clarified?  If I had to guess it would be the trans_idx line, but want to confirm.

> On the exception I was following the code that was already there, however it appears from the pytz source that it just subclasses exception so I will enhance the message.

Thanks - there are a bunch of Exception messages that could do with a rewording - it's much more helpful to have a  useful message.

> I was planning on adding the docs, both the api and tz handling sections. Also, it seems like the api docs are lacking mention of DatetimeIndex. Is that intentional or just waiting for somebody to add it?

I'm pretty sure it's not intentional. If you want to contribute docs for that - it would be great (though maybe you want to put that in a separate PR, just for clarity).

> On your comment above about adding a comment on the line, was there something in particular you want clarified? If I had to guess it would be the trans_idx line, but want to confirm.

I just was following everything up to that point - it combines so many things that it's unclear to me. Your call if you want to add a comment explaining what it's doing or not [definitely would want a different set of eyes than mine to look at it then]. I mean this line: `trans_idx = np.squeeze(np.nonzero(np.logical_and(both_nat, ~both_eq)))`

heh, I got the line wrong too - meant this one `trans_grp = np.array_split(trans_idx, np.where(np.diff(trans_idx)!=1)[0]+1)`

Also - can you hook up Travis to this? https://github.com/pydata/pandas/blob/master/CONTRIBUTING.md#steps-to-enable-travis-ci

@jtratner - incorporated your comments.  I wasn't sure how to add another pull request for the DatetimeIndex documentation (having an additional branch seemed excessive).  Also, I couldn't think of more tests where the fall transition would be obvious without already having a tz-aware index, in which case it could never be localized.  I'm open to suggestions for that.  As far as I know I added travis, but perhaps I'm mistaken.

@rockg you got Travis working :) and it definitely makes sense to add the docs with this PR.  I'll take a look when I get a chance.

Hey @rockg + @jtratner

@rockg nice start to this! I had a `.md` doc going that was attempting to explain how we should be able to infer from a time series for the same reason... just no time to implement! I looked through your source and tests and had just a few thoughts / questions.

### the `tz_localize` error

Attempts to localize time `DatetimeIndex` with `df.index.tz_localize(tz)` raise  both `NonExistentTimeError`, and `AmbiguousTimeError`, (both sub classes of `pytz.InvalidTimeError` base class) and arise for different reasons.
- Your PR seems like it only addresses the `AmbiguousTimeError` encountered during DST Fall transition, is that correct? I also have use cases in parsing time series from a local tz when I hit an `NonExistentTimeError` for local timestamps formats that can be encountered during a DST Spring transition. For example:
  
  ``` python
  import pytz
  tz = pytz.timezone('US/Eastern')
  tz.localize(pd.datetime(2012, 3, 11, 2, 00), is_dst=None)
  ```
  
  raises  `NonExistentTimeError`. This is due to `pyzt`'s terse interpretation of the Olsen timezone database, which states, explicitly, that the DST transition occurs at `02:00:00`, (only allowing for timestamps advances like:  `01:59:59` to `03:00:00`). Thus, any formatting practice that instead performs an advance like `02:00:00` to `03:00:01`, fails. This might seem like a unique case, but trust me, in my line of work, this type of local timestamp formatting can be found everywhere. :(
- Does the PR handle timestamp frequencies less than hourly? In this case, the repeated timestamp count would be equal to the number of sub-hourly frequency (I saw a control flow for number of repeated hours?).

Hope that all makes sense! Let me know if you have any questions. Thanks for tackling this, it's going to be very useful!

You are correct that this only addresses the Ambiguous error that occurs in the fall transition.  I understand your example (it's second-ending time rather than second-beginning, e.g., the time from 1:59:59 or 2:00:00 can really be represented as either 1:59:59 or 2:00:00 as it is really representing the span between).  This is always a choice when determining how to represent times and it would be nice if pytz was more flexible.  I don't have any easy thoughts here besides changing your times to be second-beginning which I'm sure you thought of.

Frequencies less than an hour are supported.  The code just looks for a negative diff in times (i.e., a time repeated in the hour) and assumes that these are for the non-dst hour.  If there is no such diff, then you still have ambiguous times.  I think this logic will work for the vast majority of the cases.  Can you point your reference to the number of repeated hours?

@rockg how's this coming ?

I believe it's all set.  I incorporated @jtratner's comments and have been using it without issue.

@nehalecky what do you think? you seem to have the best sense of this.

@rockg 
can you add release note and a mention in v0.13.0 (maybe with a doc reference to the new doc section in timeseries.rst)?

and pls squash down to a smaller number of commits

Sure (and I'll also point to the new api documentation). Is there a help on how to combine the commits?  Naively I could back out the current changes and recommit but that seems unnecessary and unwieldy. 

@rockg 

you can combine as you see fit....ideally just put the changes that go together, together. you can also put docs/tests/changes in separate. whatever seems logical.

just rebase

`git rebase -i <prior to your first comit>` then squash/fixup as needed

@rockg to be clear, this means that you'll see something like this:

```
pick 3502b3b ENH: Add 'is_' method to Index for identity checks
pick b10b44f BUG: Fix copy s.t. it always copies index/columns.
pick e3b4b59 ENH: enable deep copy for sparse Series/Panel

# Rebase b037b0e..e3b4b59 onto b037b0e
```

change "pick" to "squash" and it'll be all good.

Hey all. 

@rockg, thanks for your replies and again, nice job! While there is more that can be done with DST transition inference, I think that this is a **great** start and a very helpful feature! I look forward to its incorporation in pandas and hope to contribute to expanding its functionality in the future. :)

after you rebase if @jreback and @nehalecky think it looks good, Ithink we can merge this.

@jtratner 

Do I then "push" the rebased commits?

do

``` sh
git push --force
```

Thanks @cpcloud 

I think this is all in good order.  There are two commits, one for the new DatetimeIndex api documentation and then another for the tz_localize change.

@rockg @cpcloud this looks ok to merge, yes?

@cpcloud @jreback 

Updated commits based on comments and add Index documentation.  I still have to do the vbench but it's difficult to construct a meaningful (one that has more than 5 points).

@rockg just run the current vbench to see if there is any degradation given the changes touch some general ts code (I don't think u will see anything though)

i meant a new vbench that specifically uses `infer_dst` bc of all the calls to the numpy Python API from Cython.

@rockg ping us when ready with the vbench..thxs

@jreback Think it's set.  The documentation commit is unchanged.

@rockg can you rebase on master just to make sure.....we just merged in nanosecond support so making sure no issues

also can you post the vbench results?

@jreback Seems like rebasing is potentially destructive so want to make sure I'm doing it right.  Would it be:
1. git checkout tz-localize-aware
2. git rebase master

And for vbench, do you want the entire test set pasted here?  I ran some yesterday and noticed a couple ratios of 1.6 but it's not clear if that is a result of something unrelated to my changes or not.