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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

ENH: Pass kwargs to calendar init #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions tests/test_calendar_dispatcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@
from trading_calendars.calendar_utils import TradingCalendarDispatcher
from trading_calendars.exchange_calendar_iepa import IEPAExchangeCalendar

import pandas as pd
import pytz


class CalendarAliasTestCase(TestCase):

Expand All @@ -18,8 +21,8 @@ def setupClass(cls):
# Make a calendar once so that we don't spend time in every test
# instantiating calendars.
Copy link
Member

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?

Copy link
Collaborator Author

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.

cls.dispatcher_kwargs = dict(
calendars={'IEPA': IEPAExchangeCalendar()},
calendar_factories={},
calendars={},
calendar_factories={'IEPA': IEPAExchangeCalendar},
aliases={
'IEPA_ALIAS': 'IEPA',
'IEPA_ALIAS_ALIAS': 'IEPA_ALIAS',
Expand Down Expand Up @@ -104,3 +107,10 @@ def test_get_calendar_names(self):
sorted(self.dispatcher.get_calendar_names()),
['IEPA', 'IEPA_ALIAS', 'IEPA_ALIAS_ALIAS']
)

def test_kwarg_passing(self):
start_date = pd.Timestamp('2000-01-03', tz=pytz.UTC)
cal = self.dispatcher.get_calendar(
'IEPA', start=start_date
)
self.assertEqual(cal.first_session, start_date)
9 changes: 6 additions & 3 deletions trading_calendars/calendar_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,15 +162,18 @@ def __init__(self, calendars, calendar_factories, aliases):
self._calendar_factories = dict(calendar_factories)
self._aliases = dict(aliases)

def get_calendar(self, name):
def get_calendar(self, name, **kwargs):
"""
Retrieves an instance of an TradingCalendar whose name is given.

Parameters
----------
name : str
The name of the TradingCalendar to be retrieved.

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.
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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.

Returns
-------
calendar : calendars.TradingCalendar
Expand All @@ -191,7 +194,7 @@ def get_calendar(self, name):
raise InvalidCalendarName(calendar_name=name)

# Cache the calendar for future use.
calendar = self._calendars[canonical_name] = factory()
calendar = self._calendars[canonical_name] = factory(**kwargs)
return calendar

def get_calendar_names(self):
Expand Down