Skip to content

Do not convert series when looking at sub-daily frequencies. #8782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 14, 2014

Conversation

rockg
Copy link
Contributor

@rockg rockg commented Nov 11, 2014

Fixes #8772 by not converting values from UTC to local timezone when trying to infer sub-daily frequencies. Across a DST transition this will result in a non-unique deltas and an inferred frequency of None.

@@ -712,10 +712,18 @@ def __init__(self, index, warn=True):
@cache_readonly
def deltas(self):
return tslib.unique_deltas(self.values)

@cache_readonly
def deltas_original(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe name these deltas_i8 (or deltas_underlying), and similarly for unique.

@jreback
Copy link
Contributor

jreback commented Nov 11, 2014

pls add a release note, minor corrections, otherwise looks good

@jreback jreback added Frequency DateOffsets Timezones Timezone data dtype labels Nov 11, 2014
@jreback jreback added this to the 0.15.2 milestone Nov 11, 2014
@jreback
Copy link
Contributor

jreback commented Nov 11, 2014

cc @ischwabacher you happy with this? any other tests needed?

self.assertEqual(index.inferred_freq, "3H")

index = date_range("2014-10-25 03:00", periods=5, freq="10H", tz="Europe/Paris")
self.assertEqual(index.inferred_freq, "10H")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add this:

        index = date_range("2013-11-03", periods=5, freq="3H").tz_localize("America/Chicago")
        self.assertIsNone(index.inferred_freq)

@ischwabacher
Copy link
Contributor

Otherwise LGTM.

@rockg
Copy link
Contributor Author

rockg commented Nov 11, 2014

I am going to add more tests over both DST transitions (fall and spring) and then some more granular frequencies. This is why we are in the trouble we are in for all the DST issues...the tests don't cover the transitions.

I can add that new one as well. Will be later this evening. Thanks.

@ischwabacher
Copy link
Contributor

Yeah, I've started using ranges including 2013-11-03 or 2014-03-09 as interactive test cases whenever I start to mock up some new feature involving times in ipython. Which occasionally leads me to think "oh, I can't do that" when the problem is actually "oh, that doesn't work over a DST boundary".

Also I really like the approach taken in test_nanops.py, where a large number of arrays are constructed and then passed to each of the different tests. If we have a menagerie of weird time zone corner cases easily available, it'll make testing so much easier.

@rockg
Copy link
Contributor Author

rockg commented Nov 13, 2014

@jreback, should be good now.

@jreback jreback merged commit 90c1fc4 into pandas-dev:master Nov 14, 2014
@jreback
Copy link
Contributor

jreback commented Nov 14, 2014

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frequency DateOffsets Timezones Timezone data dtype
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'infer_freq' does not work with DST transition
3 participants