Skip to content

BUG: Fixes issue with offsetting days in days_at_time calendar helper #1333

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
Jul 18, 2016

Conversation

yankees714
Copy link
Contributor

Offsetting days in the local timezone can cause issues when there are discontinuities, such as at the start of DST. So we instead do this offset in UTC, then localize to the local timezone once dealing with times.


# Offset days in UTC to avoid discontinuities due to DST, before
# localizing to tz.
days = DatetimeIndex(days).tz_localize(None).tz_localize('UTC')
Copy link
Contributor

Choose a reason for hiding this comment

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

What bug is this fixing? Do we need a regression test for this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'm adding the test on my branch (building the test framework for the CME calendar (and others) there).

It's fixing this problem:

(Pdb++) calendar.schedule.loc['2004-04-05']
market_open    2004-04-03 23:01:00
market_close   2004-04-05 22:00:00
Name: 2004-04-05 00:00:00+00:00, dtype: datetime64[ns]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which with this change is corrected to:

(Pdb++) calendar.schedule.loc['2004-04-05']
market_open    2004-04-04 22:01:00
market_close   2004-04-05 22:00:00
Name: 2004-04-05 00:00:00+00:00, dtype: datetime64[ns]

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that a bug in this function, or are we working around a pandas bug? I don't understand how the changes here fix that problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like prior to pandas-dev/pandas#10744, there were issues with using DateOffset in this context. Timedelta seems to be a better choice.

@coveralls
Copy link

coveralls commented Jul 18, 2016

Coverage Status

Coverage increased (+0.001%) to 83.68% when pulling 222ecb1 on fix-days-at-time-offset into 5f09203 on master.

Prior to pandas 17, there were issues with offsetting dates with
DateOffset around discontinuities (like the start of DST). We can use
Timedelta here instead, which handles these edge cases.
@yankees714 yankees714 force-pushed the fix-days-at-time-offset branch from 222ecb1 to bcc867b Compare July 18, 2016 21:24
@coveralls
Copy link

coveralls commented Jul 18, 2016

Coverage Status

Coverage remained the same at 83.678% when pulling bcc867b on fix-days-at-time-offset into 5f09203 on master.

@yankees714 yankees714 merged commit dffdf0a into master Jul 18, 2016
@yankees714 yankees714 deleted the fix-days-at-time-offset branch July 18, 2016 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants