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

Cache holidays #117

Merged
merged 1 commit into from
Sep 3, 2020
Merged

Cache holidays #117

merged 1 commit into from
Sep 3, 2020

Conversation

2torus
Copy link
Contributor

@2torus 2torus commented Sep 2, 2020

Addresses #108
Cache holidays computation in order to improve running time of repeated valid_days calls.

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.

I'm not sure if the computational time bottleneck for valid_days at #108 is really CustomBusinessDay() and not pd.date_range() though. Have you checked @2torus ?

In any case, this looks like an improvement (though perhaps only partially addresses # 108) to me.

@2torus
Copy link
Contributor Author

2torus commented Sep 3, 2020

@leonarduschen Thanks for the review.

This Dockerfile builds either versions of the package and then runs a small timing script.

I got:

> docker build . -t mcal_old >/dev/null && docker run -it mcal_old
18.30
> docker build --build-arg NEW="true" . -t mcal_new >/dev/null && docker run -it mcal_new 
1.95

My experience is that 90% of the time in valid_days() is spent in holidays() in the current version.

@leonarduschen
Copy link
Contributor

Awesome. I tried caching the whole valid_days() method a few days ago by storing a very long pd.date_range() on class initialization and managed to get similar results, but I decided to scrape the idea off because it feels very inelegant!

This seems like a far superior solution, much cleaner and not much extra memory.

@2torus
Copy link
Contributor Author

2torus commented Sep 3, 2020

@leonarduschen I agree. The user could ask for 40-50 years worth of dates, you don't want to be taking the caching decision for them. Holidays, otoh, is not something you expect to be changing.

@rsheftel rsheftel merged commit 73d358e into rsheftel:master Sep 3, 2020
@rsheftel
Copy link
Owner

rsheftel commented Sep 3, 2020

Thanks for this.

rsheftel added a commit that referenced this pull request Sep 3, 2020
@2torus
Copy link
Contributor Author

2torus commented Sep 13, 2020

@leonarduschen @rsheftel Thank you for allowing me to contribute. I would love to help out in the future. Especially with the quantopian merger.

@rsheftel
Copy link
Owner

@2torus thanks for your work. Maybe a good place to start helping on the merger would be looking at the calendar and code differences and seeing if you can help determine what is the correct calendar days, or help with adding the functionality to trading_calendars to close the gap.

quantopian/trading_calendars#152

quantopian/trading_calendars#151

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.

3 participants