-
Notifications
You must be signed in to change notification settings - Fork 382
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: Pass kwargs to calendar init #176
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gerrymanoim ! Small PR fueled a bunch of thoughts...
@@ -18,8 +21,8 @@ def setupClass(cls): | |||
# Make a calendar once so that we don't spend time in every test | |||
# instantiating calendars. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this isn't true anymore. Do we no longer have that speed concern and/or shall we update this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep - I should update this comment.
kwargs: Dict[str, Any] | ||
Optional keyword args passed to calendar `__init__. | ||
Note: Arguments are only passed when the calendar is first | ||
constructed. Subsequent calls will not affect the calendar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confusion is my biggest concern. What do you think about a warning for subsequent calls with kwargs? Or warn if they don't match the original? Or create a new calendar if it doesn't match?
Maybe it would be clearer for people to use the more obvious mutator register_calendar('XNYS', XNYSExchangeCalendar(start=my_start), force=True)
or register_calendar_type('XNYS', partial(XNYSExchangeCalendar, start=my_start), force=True)
to override the default themselves? Or another idea: Move the start_default
from the module to the dispatcher, and let callers set it, invalidating the cache.
Having get_calendar
potentially create a calendar definitely makes lifecycle/state management more challenging here...
Looking at that original zipline issue, where should someone call get_calendar
with a non-default start
to be able to load earlier data into their csv bundle? Will they have a chance before the internals of the zipline
cli calls get_calendar
with the default start
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see the argument for moving start_default
/end_default
to the dispatcher and allowing you to change that globally (as properties on the dispatcher instance?), but I think the interface in this PR targets the most common use case (though I could be mistaken) when you're grabbing a calendar out of a registry. I wanted to keep it very straightforward to pull out preexisting calendars. I don't want to force users to go re-register a bunch of calendars.
I can definitely add a warning here if you don't think that would be too noisy.
Looking at that original zipline issue
I think zipline should either pass the dates it wants here (i.e. passing the start
in https://github.com/quantopian/zipline/blob/master/zipline/__main__.py#L290) or we should clip the data bundles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think zipline should either pass the dates it wants here (i.e. passing the start in https://github.com/quantopian/zipline/blob/master/zipline/__main__.py#L290) or we should clip the data bundles.
If the expectation is that that particular call site will actually construct the calendar, that feels brittle to me, since it's not enforced. Every third-party extension or zipline module will have to be sure not to call get_calendar
first. For this case, have you confirmed that that is the first call site to run (at least on master currently without extensions)? We could solve that potentially with a cli unit test, but my preference is to have more robust underpinnings, since I'd hate to cause more confusion when writing an extension, i.e. if you call get_calendar
too early, it will mess zipline up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the expectation is that that particular call site will actually construct the calendar, that feels brittle to me, since it's not enforced.
Then should we have zipline not use the registry? I can add some sort of no_cache
option to get_calendar
as well so zipline can forcibly recreate the calendar it needs to, but I'm not sure using the global dispatcher is right then.
In general, I feel that we should lean towards making the standalone usage of trading_calendars
more ergonomic and making changes in zipline we need to in order to support that (though this PR would keep zipline working as is).
For this case, have you confirmed that that is the first call site to run (at least on master currently without extensions)?
If you use the cli main entry point, that's the first instance we grab the calendar.
Update readme Makes changes to readme advising of: - 4.0 development. - New library market_prices.
Update readme Makes changes to readme advising of: - 4.0 development. - New library market_prices.
Closes #147.
Allows you to pass
kwargs
to calendars viaget_caledar
, i.e.: