-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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/ENH: Allow users to switch between calendars #1800
Conversation
bc60606
to
046c72f
Compare
046c72f
to
475c25a
Compare
475c25a
to
63e11d4
Compare
zipline/algorithm.py
Outdated
self.trading_calendar = get_calendar("NYSE") | ||
else: | ||
self.trading_calendar = get_calendar(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.
I'm pretty sure that if I'm using kwargs.pop()
above that I can remove the if
statement and just assign self.trading_calendar
to get_calendar(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.
Yup, you've encapsulated the logic of the conditional statement in the pop
: if trading_calendar
is not in the kwargs
then use "NYSE"
.
63e11d4
to
3146de4
Compare
f72f584
to
fdb50eb
Compare
2 similar comments
@yankees714 could you take a look at this? I was wondering if/where I should have more test coverage now that we're adding/fixing this. |
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 is a great change! Had some questions and suggestions for you.
Regarding tests, from what I can tell we don't have much for the CLI or run_algorithm
. I do see one call to run_algorithm
, so we at least get some coverage there of threading the new arg.
zipline/__main__.py
Outdated
'--trading-calendar', | ||
metavar='TRADING-CALENDAR', | ||
default='NYSE', | ||
help="The calendar you want to use e.g. LSE. NYSE is the default." |
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.
Thoughts on making this a choice option? http://click.pocoo.org/5/options/#choice-options
You could use the keys of _default_calendar_factories
in zipline.utils.calendars.calendar_utils
to populate it
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.
TIL. Going to make that change right now
def create_simulation_parameters(year=2006, start=None, end=None, | ||
def create_simulation_parameters(year=2006, | ||
start=None, | ||
end=None, |
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.
👍
zipline/testing/fixtures.py
Outdated
@@ -598,6 +598,8 @@ class WithSimParams(WithTradingEnvironment): | |||
SIM_PARAMS_START = alias('START_DATE') | |||
SIM_PARAMS_END = alias('END_DATE') | |||
|
|||
SIM_PARAMS_CALENDAR = get_calendar('NYSE') |
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.
What's this change for?
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 noticed that we were only using cls.trading_calendar
for testing; figured I'd switch it to just use NYSE
. But now that you've pointed it out, we'd probably not want this change so we can test across whatever calendar is passed in
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.
Ah I also saw test failures because the default trading_calendar
is None:
e.g.
======================================================================
FAIL: test_schedule_function_time_rule_positionally_misplaced (tests.test_algorithm.TestAlgoScript)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/freddiev4/Documents/quantopian/zipline/tests/test_algorithm.py", line 2272, in test_schedule_function_time_rule_positionally_misplaced
data_frequency='minute'
File "/Users/freddiev4/Documents/quantopian/zipline/zipline/utils/factory.py", line 62, in create_simulation_parameters
trading_calendar=trading_calendar,
File "/Users/freddiev4/Documents/quantopian/zipline/zipline/finance/trading.py", line 141, in __init__
"Must pass in trading calendar!"
AssertionError: Must pass in trading 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.
I'm not sure that failure is related, since that test is calling create_simulation_parameters
directly, not using the self.sim_params
attribute provided by the fixture
@@ -155,15 +156,20 @@ def choose_loader(column): | |||
env = TradingEnvironment(environ=environ) | |||
choose_loader = None | |||
|
|||
if not trading_calendar: | |||
trading_calendar = get_calendar('NYSE') |
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 we're already doing this in create_simulation_parameters
here, so I'm thinking we don't need handling at this level?
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.
Both TradingAlgorithm
and SimulationParameters
take a TradingCalendar
object so I added those lines to get the calendar object specifically for TradingAlgorithm
, IIRC (but thinking about it now, it doesn't make sense)
If anything, I'm thinking I could just remove the handling from create_simulation_parameters
, because I'm just using the same object here for both trading algo and simparams.
Thoughts?
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.
Ah, makes sense that we need to pass an actual TradingCalendar
when constructing the TradingAlgorithm
. Seems like we're using create_simulation_parameters
a bunch of places in the tests where we don't pass a calendar, so probably fine to leave this as-is
@@ -60,3 +60,6 @@ piprot==0.9.6 | |||
|
|||
# For mocking out requests fetches | |||
responses==0.4.0 | |||
|
|||
# Debugging | |||
pdbpp==0.9.1 |
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.
Should we pin pdbpp's dependencies as well?
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.
Sure I think that's fair to do
You previously could not switch between different calendars because the `trading_calendar` parameter was not being passed into run_algo or `TradingAlgorithm`. You now can pass which calendar you'd like to use via the CLI using `--trading-calendar`.
fdb50eb
to
eb8ddbe
Compare
1 similar comment
@yankees714 I think this should be good for a 2nd review |
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.
LGTM! Just had one more style suggestion
zipline/__main__.py
Outdated
from zipline.data import bundles as bundles_module | ||
from zipline.utils.calendars.calendar_utils import ( | ||
get_calendar, | ||
_default_calendar_factories |
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.
We probably don't want to keep the leading _
if we're importing this here.
Better yet, what would you think of including a line in calendar_utils
like
default_calendar_names = _default_calendar_factories.keys()
and importing that instead?
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.
Sounds good to me. I also wrapped it with sorted()
1 similar comment
Hi @freddiev4 , I am new to zipline. I am trying to use a 24/7 calendar, so I am trying to use the feature of this PR already to do it in a proper way. I am using zipline installed via pip directly from github's egg (`pip install git+git://github.com/quantopian/zipline') Unfortunately I am not able to use this feature properly, here is what I am doing: I edited the # A bunch of imports removed for the sake of clarity
class TwentyFourSevenCalendar(TradingCalendar):
"""
Exchange calendar for 24/7 trading.
Open Time: 12am, UTC
Close Time: 11:59pm, UTC
"""
@property
def name(self):
return "TWENTYFOURSEVEN"
@property
def tz(self):
return timezone("UTC")
@property
def open_time(self):
return time(0, 0)
@property
def close_time(self):
return time(23,59)
@lazyval
def day(self):
return CustomBusinessDay(
weekmask='Mon Tue Wed Thu Fri Sat Sun',
)
start_session = pd.Timestamp('2014-01-07', tz='utc')
end_session = pd.Timestamp('2017-11-13', tz='utc')
assets = ['BTC_USD']
register_calendar(
'TWENTYFOURSEVEN',
TwentyFourSevenCalendar(
start=start_session,
end=end_session
)
)
register(
'custom-csvdir-bundle',
csvdir_equities(["daily"], '/home/av/repos/ziplinetest/csvdir'),
calendar_name='TWENTYFOURSEVEN',
start_session=start_session,
end_session=end_session
) As you can see I am using csvdir bundle which has recently been merged. The ingestion process seems to work fine (no errors and I also verified the directory and the sqlite files exist, so I assume the ingestion is ok). I conclude therefore that the 24/7 calendar works properly during ingestion. When I try to execute with: zipline run -f csvdir_example.py \
-b custom-csvdir-bundle \
--start 2016-1-1 \
--end 2017-1-1 \
--data-frequency daily \
--trading-calendar TWENTYFOURSEVEN \
--capital-base 100000 \
-o csvdir_example.pickle I get:
Could you please help me? Thanks a lot. |
You previously could not switch between different calendars because the
trading_calendar
parameter was not being passed into run_algo orTradingAlgorithm
. Once merged, you should be able to pass which calendar you'd like to use via the CLI using--trading-calendar
.e.g.
zipline run -f algo.py --start 2012-01-01 --end 2013-01-01 --trading-calendar LSE
Caveats:
TradingAlgorithm
andSimulationParameters
both take atrading_calendar
object as arguments. Rather than going down the rabbit hole of changing up the implementation of both of those classes, I decided to just allow users to first have the capability of using different calendars, and set aside the re-implementation issue as a separate project at a later point.TradingEnvironment
also takes atrading_calendar
object