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

MAINT: Pass calendars to DataPortal #2026

Merged
merged 1 commit into from Nov 30, 2017

Conversation

Projects
None yet
4 participants
@freddiev4
Contributor

freddiev4 commented Nov 28, 2017

Following the posts in #2018, should fix that issue

When we enter the DataPortal() setup in zipline/zipline/data/data_portal.py(296), and call

# Get the first trading minute
self._first_trading_minute, _ = (
    self.trading_calendar.open_and_close_for_session(
    self._first_trading_day
)

We error at this point, if we're using a custom TradingCalendar because we actually only pass the NYSE calendar to the DataPortal() in zipline/utils/run_algo.py , so I changed the line to pass the calendar received from the CLI:

--- a/zipline/utils/run_algo.py
+++ b/zipline/utils/run_algo.py
@@ -134,7 +134,8 @@ def _run(handle_data,
         first_trading_day =\
             bundle_data.equity_minute_bar_reader.first_trading_day
         data = DataPortal(
-            env.asset_finder, get_calendar("NYSE"),
+            env.asset_finder,
+            trading_calendar=trading_calendar,
             first_trading_day=first_trading_day,
             equity_minute_reader=bundle_data.equity_minute_bar_reader,
             equity_daily_reader=bundle_data.equity_daily_bar_reader,

@freddiev4 freddiev4 force-pushed the data-portal-likes-calendars branch from 6fa8f6f to d15270b Nov 30, 2017

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2017

Coverage Status

Coverage increased (+0.002%) to 87.374% when pulling d15270b on data-portal-likes-calendars into 49a3a4a on master.

@freddiev4 freddiev4 requested review from yankees714 and richafrank Nov 30, 2017

@richafrank

Looked good to me. Had one request.

@@ -154,7 +154,11 @@ def __init__(self,
minute_history_prefetch_length=_DEF_M_HIST_PREFETCH,
daily_history_prefetch_length=_DEF_D_HIST_PREFETCH):
self.trading_calendar = trading_calendar
if trading_calendar is not None:

This comment has been minimized.

@richafrank

richafrank Nov 30, 2017

Member

It looks like we do this check in run_algo._run as well. Since that's the only call site that now might pass a None trading_calendar, can we do the check just there instead of here too?

@freddiev4 freddiev4 force-pushed the data-portal-likes-calendars branch 2 times, most recently from 0459db3 to 594fc0b Nov 30, 2017

@freddiev4 freddiev4 force-pushed the data-portal-likes-calendars branch from 594fc0b to d5855ec Nov 30, 2017

@yankees714

LGTM, but had a thought on how we could save the list of choices in the CLI help.

@@ -54,7 +54,7 @@
MinuteHistoryLoader,
)
from zipline.data.us_equity_pricing import NoDataOnDate
from zipline.utils.calendars.calendar_utils import get_calendar

This comment has been minimized.

@yankees714

yankees714 Nov 30, 2017

Contributor

I think this is cruft?

@@ -179,7 +176,6 @@ def _(*args, **kwargs):
@click.option(
'--trading-calendar',
metavar='TRADING-CALENDAR',
type=click.Choice(default_calendar_names),

This comment has been minimized.

@yankees714

yankees714 Nov 30, 2017

Contributor

I kind of like including all of the options here. Looking at TradingCalendarDispatcher, it seems like we're actually indirectly defining what we want here in the has_calendar method:

def has_calendar(self, name):
"""
Do we have (or have the ability to make) a calendar with ``name``?
"""
return (
name in self._calendars
or name in self._calendar_factories
or name in self._aliases
)

What would you think of splitting out an available_calendar_names property that just returns the union of what we check against in has_calendar? Then we could use that in the definition of has_calendar, and also expose it for the CLI.

This comment has been minimized.

@richafrank

richafrank Nov 30, 2017

Member

Have we loaded all the user extensions by this point in time, e.g. at zipline import time?

This comment has been minimized.

@yankees714

yankees714 Nov 30, 2017

Contributor

Oh good point, not necessarily 😞

Probably worth just leaving this out then.

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2017

Coverage Status

Coverage remained the same at 87.372% when pulling d5855ec on data-portal-likes-calendars into 49a3a4a on master.

@richafrank

LGTM

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Nov 30, 2017

@richafrank should be good to go now (?)

EDIT: You beat me by 19 seconds

@freddiev4 freddiev4 merged commit 42b3200 into master Nov 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@freddiev4 freddiev4 deleted the data-portal-likes-calendars branch Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment