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

use trading_calendars in zipline #2219

Merged
merged 2 commits into from Jul 2, 2018

Conversation

Projects
None yet
4 participants
@vikram-narayan
Contributor

vikram-narayan commented Jun 22, 2018

Replace zipline.utils.calendars with trading_calendars.

@vikram-narayan vikram-narayan requested a review from ssanderson Jun 22, 2018

@vikram-narayan vikram-narayan changed the title from Use trading calendars to WIP: use trading calendars Jun 22, 2018

@coveralls

This comment has been minimized.

coveralls commented Jun 25, 2018

Coverage Status

Coverage decreased (-0.9%) to 85.445% when pulling 45fb9ba on use-trading-calendars into 24d56b7 on master.

@vikram-narayan vikram-narayan force-pushed the use-trading-calendars branch 2 times, most recently from 506b9cf to 16dc5d7 Jun 25, 2018

@vikram-narayan vikram-narayan changed the title from WIP: use trading calendars to use trading_calendars in zipline Jun 26, 2018

@vikram-narayan

This comment has been minimized.

Contributor

vikram-narayan commented Jun 26, 2018

@ssanderson mind taking a look?

@ssanderson

@vikram-narayan the changes here generally seem reasonable. A few general comments:

  1. It looks like some of the functions we copied to zipline have been deleted here but others haven't (e.g., it looks like we copied preprocess but moved lazyval, coerce, and days_at_time. It seems like we should be consistent one way or another here.
  2. I'm not sure if we want to have a single global registry for all consumers of trading_calendars, or if we want a zipline-specific registry (multipledispatch, which we use in various places, has a concept of a dispatch namespace, which you can use to have multiple distinct copies of multiply-dispatched functions with the same name. The analogy here would be if you wanted multiple different calendars registered under the same name in different parts of your program. I can't actually think of a use-case where that would be useful, so my inclination for now is to have the single global registry, but I'd be interested to get other opinions on this.
  3. I'm 50/50 on whether it's worth the source code churn to update all the imports here. This is another case where I'd be interested in a second opinion.
@@ -0,0 +1,11 @@
# flake8: noqa

This comment has been minimized.

@ssanderson

ssanderson Jun 26, 2018

Member

One thing I'm not sure of is whether we want to re-export the global registry from trading_calendars or if we want Zipline to maintain its own distinct registry.

This comment has been minimized.

@vikram-narayan

vikram-narayan Jun 26, 2018

Contributor

Like you I don't know of a use case off the top of my head where we'd need this; presumably if the calendars are different, we'd want to register them differently. Do you think it's still worth adding the multipledispatch logic in case we need it later?

This comment has been minimized.

@ssanderson

ssanderson Jun 29, 2018

Member

I think we can probably punt on this until we have a use-case for it.

@@ -21,6 +21,7 @@
from six import iteritems, string_types, PY3
from toolz import valmap, complement, compose
import toolz.curried.operator as op
from trading_calendars.trading_calendar import coerce

This comment has been minimized.

@ssanderson

ssanderson Jun 26, 2018

Member

How are we using coerce in trading_calendar if preprocess is still defined in zipline?

This comment has been minimized.

@ssanderson

ssanderson Jun 26, 2018

Member

Assuming we're still using it, trading_calendars.trading_calendar seems like an odd place for this to live. My inclination would honestly be to put the stuff we ported from zipline in something that makes that clear like trading_calendars._from_zipline or something like that.

This comment has been minimized.

@vikram-narayan

vikram-narayan Jun 26, 2018

Contributor

yup definitely makes sense to move these to a better location. if we're planning to keep these functions here permanently, should these be in trading_calendars.utils or something? might be a bit confusing to see _from_zipline in a repo that isn't zipline, that is used by zipline

This comment has been minimized.

@vikram-narayan

vikram-narayan Jun 26, 2018

Contributor

opened a PR in trading_calendars putting these functions in utils: quantopian/trading_calendars#4, lmk what you think about those locations

This comment has been minimized.

@ssanderson

ssanderson Jun 29, 2018

Member

Those seem fine. I think at some point we'll want to push preprocess into its own library, but we can save that for another day.

@@ -36,7 +36,7 @@
)
from zipline.utils.cache import dataframe_cache
from zipline.utils.functional import apply
from zipline.utils.calendars import TradingCalendar, get_calendar
from trading_calendars import TradingCalendar, get_calendar

This comment has been minimized.

@ssanderson

ssanderson Jun 26, 2018

Member

I'm 50/50 on whether it's worth changing all these imports to point to the new repo, or if we should just leave all of this the same (which should still work since we're re-exporting). Might be worth asking around internally to see if people have preferences about this.

This comment has been minimized.

@llllllllll

llllllllll Jun 26, 2018

Member

I think we should import from the real place, but move the import to be in the 3rd partly lib section. The re-export should continue to work for a while or we will break a lot of code in our data processing projects.

This comment has been minimized.

@vikram-narayan

vikram-narayan Jun 26, 2018

Contributor

asked around and people prefer importing from trading_calendars; moved imports to 3rd party section

@@ -111,10 +111,6 @@ def window_specialization(typename):
'zipline.data._minute_bar_internal',
['zipline/data/_minute_bar_internal.pyx']
),
Extension(

This comment has been minimized.

@ssanderson
@ssanderson

This comment has been minimized.

Member

ssanderson commented Jun 29, 2018

LGTM once the buidl passes.

vikram-narayan added some commits Jun 19, 2018

MAINT: use trading_calendars in zipline
- remove zipline.utils.calendars
- update imports
- add trading-calendars to requirements

@vikram-narayan vikram-narayan force-pushed the use-trading-calendars branch from 47e3fc1 to 45fb9ba Jul 2, 2018

@vikram-narayan vikram-narayan merged commit f61d3df into master Jul 2, 2018

2 checks passed

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

@vikram-narayan vikram-narayan deleted the use-trading-calendars branch Jul 2, 2018

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