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

BUG: Do not loop-back when count is greater than historical dates #125

Merged
merged 1 commit into from Sep 2, 2020

Conversation

pavitrakumar78
Copy link
Contributor

Made sure that end_idx in sessions_window method is always positive so that it does not loop-back when indexing on all_sessions when the count/lookback period is greater than the length of total historical dates. More details in #124.

Made sure that `end_idx` in `sessions_window` method is always positive so that it does not loop-back when indexing on all_sessions when the count/lookback period is greater than the length of total historical dates. More details in quantopian#124.
Copy link
Contributor

@leonarduschen leonarduschen left a comment

Choose a reason for hiding this comment

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

Isn't this good to go?

Adding end_idx = max(0, end_idx) for handling cases where count << 0 is consitent with how the function handles cases where count >> 0 (i.e. just return the slice that satisfy the requirement and ignore the excess counts).

If we'd like to raise ValueError instead as @gerrymanoim suggested on #124 then IMO it'd have to be implemented both ways. More specifically, when end_idx < 0 and when end_idx > len(self.schedule). Let me know if this is the preferred behavior, I can create some new tests then make a separate PR for that :)

@gerrymanoim gerrymanoim changed the title Update trading_calendar.py BUG: Do not loop-back when count is greater than historical dates Sep 2, 2020
@gerrymanoim
Copy link
Collaborator

Closes #124

@gerrymanoim gerrymanoim merged commit d599ea3 into quantopian:master Sep 2, 2020
sercant pushed a commit to AlpacaTechJP/trading_calendars that referenced this pull request Jan 17, 2022
XKRX: add krx holidays in 2022 and 2021 temporary holidays
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.

None yet

3 participants