Skip to content
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

ENH: Add breaks to TradingCalendar #154

Merged
merged 41 commits into from Oct 9, 2020
Merged

ENH: Add breaks to TradingCalendar #154

merged 41 commits into from Oct 9, 2020

Conversation

gerrymanoim
Copy link
Collaborator

@gerrymanoim gerrymanoim commented Sep 9, 2020

Adds break_start_times and break_end_times to TradingCalendar to model intraday lunch breaks. These work as vectors of values, similarly to how open_times and close_times work so we can model break times changing over time.

Threads the breaks through necessary methods.

Adds breaks to XHKG.

Items with TODO I'd especially like feedback on

@github-actions github-actions bot added the enhancement New feature or request label Sep 9, 2020
@gerrymanoim
Copy link
Collaborator Author

FYI @rsheftel - breaks here should be functionally similar to pandas_market_calendars but work slightly differently internally.

@gerrymanoim gerrymanoim marked this pull request as ready for review September 15, 2020 19:29
@rsheftel
Copy link
Collaborator

@gerrymanoim you may want to add the lunch break to the XSHG calendar as well. It is 11:30 AM - 1:00 PM Asia/Shanghai

@gerrymanoim
Copy link
Collaborator Author

I was going to go through and add lunch breaks after the functional pieces of this PR were approved, happy to take care of it.

@rsheftel
Copy link
Collaborator

@gerrymanoim makes sense. Once this lands on master I will look to add the CME specific calendars that require this feature.

Copy link
Member

@richafrank richafrank left a comment

Choose a reason for hiding this comment

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

Assuming the boundary conditions and any other issues are all set, I'm a bit concerned about this as a behavior change for consumers (of all_minutes, is_open_on_minute, minute_to_session_label, etc.). Should we try this out with any tests downstream?

trading_calendars/trading_calendar.py Outdated Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
trading_calendars/exchange_calendar_xhkg.py Show resolved Hide resolved
trading_calendars/calendar_helpers.py Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
return False
else:
return True

else:
try:
# if they are the same, it might be the first minute of a
Copy link
Member

Choose a reason for hiding this comment

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

What is this case? Does it apply to the re-open minute after the break?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is now handled in the check above:

if break_start_on_open_dt <= minute_val < break_end_on_open_dt:
            # we're in the middle of a break
            return False
        else:
            return True

If minute_val==break_end_on_open_dt we'll return True.

trading_calendars/trading_calendar.py Show resolved Hide resolved
gerrymanoim and others added 2 commits September 17, 2020 16:20
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
@gerrymanoim
Copy link
Collaborator Author

Should we try this out with any tests downstream?

What do you mean by that? There's a test in XHKG that tests if breaks are working correctly.

gerrymanoim and others added 7 commits September 17, 2020 17:13
Copy link
Member

@richafrank richafrank left a comment

Choose a reason for hiding this comment

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

Nice improvements! Added some suggestions. The preprocess change looked like the only breaking change.

trading_calendars/trading_calendar.py Outdated Show resolved Hide resolved
trading_calendars/trading_calendar.py Outdated Show resolved Hide resolved
trading_calendars/trading_calendar.py Outdated Show resolved Hide resolved
trading_calendars/trading_calendar.py Outdated Show resolved Hide resolved
trading_calendars/calendar_helpers.py Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
trading_calendars/calendar_helpers.py Outdated Show resolved Hide resolved
trading_calendars/trading_calendar.py Show resolved Hide resolved
tests/test_trading_calendar.py Outdated Show resolved Hide resolved
tests/test_trading_calendar.py Outdated Show resolved Hide resolved
gerrymanoim and others added 15 commits October 8, 2020 16:41
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Co-authored-by: Richard Frank <richafrank@users.noreply.github.com>
Copy link
Member

@richafrank richafrank left a comment

Choose a reason for hiding this comment

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

Nice! Just had one suggestion.

trading_calendars/trading_calendar.py Show resolved Hide resolved
@gerrymanoim gerrymanoim merged commit 81b1acf into master Oct 9, 2020
@gerrymanoim gerrymanoim deleted the time-for-lunch branch October 9, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants