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: Futures API #637

Merged
merged 1 commit into from Jul 13, 2015

Conversation

Projects
None yet
3 participants
@yankees714
Contributor

yankees714 commented Jul 7, 2015

Implements a user facing futures API for zipline.

Adds the FutureChain object, which allows users to access future contracts though underlying queries to the AssetFinder. Adds the API function future_chain, which returns FutureChain objects.

@yankees714

This comment has been minimized.

Contributor

yankees714 commented Jul 7, 2015

@jfkirk I still need to flesh out some of the tests on this, but it'd be great to have your input on where things are right now.

def test_root_symbols(self):
""" Test that different variations on root symbols are handled
as expcted.

This comment has been minimized.

@jfkirk

jfkirk Jul 7, 2015

Contributor

typo

algo.cl.offset('3 days').as_of_date,
(pd.tseries.tools.normalize_date(algo.sim_params.period_start)
+ pd.Timedelta('3 days'))
)

This comment has been minimized.

@jfkirk

jfkirk Jul 7, 2015

Contributor

Should we test that offset works both forwards and backwards in time?

def tearDown(self):
teardown_logger(self)
def test_len(self):

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

@yankees714 why do we need to be running full simulations to test this functionality?

This comment has been minimized.

@yankees714

yankees714 Jul 7, 2015

Contributor

@ssanderson I think it makes sense to do at least some of it this way, since a lot of things are dependent on dates getting set (e.g. caching the current chain, getting the initial date from the trading algorithm). But I agree that a lot of this could be done outside of a simulation

# If an as_of_date is provided, self._as_of_date uses that
# value, otherwise None. This attribute backs the as_of_date property.
if as_of_date:
self._as_of_date = (TradingEnvironment.instance().normalize_date(

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

You don't need the TradingEnvironment to do this. TradingEnvironment.normalize_date just does:

    def normalize_date(self, test_date):
        test_date = pd.Timestamp(test_date, tz='UTC')
        return pd.tseries.tools.normalize_date(test_date)
as_of_date : pandas.Timestamp
"""
def __init__(self, trading_algorithm, root_symbol='', as_of_date=None):

This comment has been minimized.

@jfkirk

jfkirk Jul 7, 2015

Contributor

When would root_symbol ever be ''?

This comment has been minimized.

@yankees714

yankees714 Jul 7, 2015

Contributor

@jfkirk I think we had discussed at some point that root_symbol should be a keyword arg. Maybe it should just be positional, since there's really no sensible default case?

This comment has been minimized.

@jfkirk

jfkirk Jul 8, 2015

Contributor

It should be a kwarg, as it is, but I don't think it should have a default value. Failing to pass a root_symbol should be a failure case.

if dt is None:
dt = self._trading_algorithm.sim_params.period_start
return TradingEnvironment.instance().normalize_date(dt)

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

c.f. comment above

# Maintain a reference to the TradingAlgorithm which holds this
# FutureChain instance, so we can get its datetime.
self._trading_algorithm = trading_algorithm

This comment has been minimized.

@jfkirk

jfkirk Jul 7, 2015

Contributor

This may get real interesting with serialization. I don't know how we can improve this situation, but if there is one thing I'd like to change, it is how we hook-in to the algo's dt.

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

I'm strongly of the opinion that we shouldn't be passing around full and storing a full TradingAlgorithm instance here. Doing so makes this class much harder to test, and effectively makes all of Zipline a dependency.

if (self._last_updated is None or
(self._last_updated != self._get_datetime())):
asset_finder = TradingEnvironment.instance().asset_finder

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

Can we just pass in the asset_finder on construction rather than pulling it out of a global every time this is called?

return TradingEnvironment.instance().normalize_date(dt)
@property
def as_of_date(self):

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

Having both an as_of_date property and an as_of method is a little confusing.

"""
def __init__(self, trading_algorithm, root_symbol='', as_of_date=None):
self.root_symbol = root_symbol.upper()

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

This conversion should probably be done by the context that constructs the FutureChain, not by the chain itself.

----------
trading_algorithm : TradingAlgorithm
root_symbol : str, optional
as_of_date : pandas.Timestamp

This comment has been minimized.

@jfkirk

jfkirk Jul 7, 2015

Contributor

Can you add quick descriptions of the parameters?

def __repr__(self):
if self._as_of_date:
return 'FutureChain(root_symbol=\'%s\', as_of_date=\'%s\')' % (

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

Rather than escape the single quotes here, you should just use double quotes around the outer string.

In [6]: "This is a string with 'quotes'"
Out[6]: "This is a string with 'quotes'"
return 'FutureChain(root_symbol=\'%s\', as_of_date=\'%s\')' % (
self.root_symbol, self.as_of_date)
else:
return 'FutureChain(root_symbol=\'%s\')' % self.root_symbol

This comment has been minimized.

@ssanderson
# Params and trade source for a 50 day simulation
self.sim_params_50 = factory.create_simulation_parameters(num_days=50)
self.source_50 = factory.create_minutely_trade_source(

This comment has been minimized.

@ssanderson

ssanderson Jul 7, 2015

Member

Independently of whether we decide to test FutureChain by running full simulations (I'm not convinced that's necessary), a 50 day simulation at minutely frequency is WAY overkill. The speed of the test suite matters a lot for developer productivity; we should be doing the minimum amount of work required to fully test the desired functionality.

This comment has been minimized.

@yankees714

yankees714 Jul 8, 2015

Contributor

I'm making most of these directly test the FutureChain object, outside of simulation. Then we'll just need a few tests for the actual future_chain API function.

@yankees714 yankees714 force-pushed the futures-api branch 5 times, most recently from e9cb993 to 79f6045 Jul 8, 2015

@yankees714

This comment has been minimized.

Contributor

yankees714 commented Jul 8, 2015

@jfkirk @ssanderson I cleaned things up a bunch based on your suggestions, also improved the tests. Mind giving this a look again?

)
@property
def as_of_date(self):

This comment has been minimized.

@yankees714

yankees714 Jul 9, 2015

Contributor

Do we think having this property and the as_of(time_delta) method is confusing enough to merit a name change? I think we should keep the as_of syntax for the user i.e. future_chain('CL').as_of(...).

This comment has been minimized.

@jfkirk

jfkirk Jul 9, 2015

Contributor

Not to back-track the conversation, but I'm not totally clear on why we support this pattern:

future_chain('CL').as_of(dt)

When, in my mind, this pattern is clearer to me:

future_chain('CL', as_of=dt)

This comment has been minimized.

@yankees714

yankees714 Jul 9, 2015

Contributor

I think the idea is that you'd be able to do something like:

def initialize(context):
    context.cl = future_chain('CL')

def handle_data(context, data):
    # ...
    context.cl.as_of(dt)...

This comment has been minimized.

@yankees714

This comment has been minimized.

@ssanderson

ssanderson Jul 10, 2015

Member

@jfkirk the example @yankees714 posted is what I had in mind. Being able to say "interpret FutureChain X in another as_of" seems like a useful idiom.

This comment has been minimized.

@jfkirk

jfkirk Jul 10, 2015

Contributor

Yep - makes sense. Thanks!

----------
root_symbol : str
The root symbol of the future chain.
as_of_date

This comment has been minimized.

@jfkirk

jfkirk Jul 9, 2015

Contributor

unfinished comment?

This comment has been minimized.

@yankees714

yankees714 Jul 9, 2015

Contributor

Since this is a property and has its own docstring the numpy conventions suggest to just include it by name
https://github.com/numpy/numpy/blob/master/doc/HOWTO_DOCUMENT.rst.txt#class-docstring

This comment has been minimized.

@ssanderson

ssanderson Jul 10, 2015

Member

I think a one-line summary would be fine here too.

@yankees714 yankees714 force-pushed the futures-api branch 6 times, most recently from f060f73 to be93dea Jul 9, 2015

Raises
------
RootSymbolNotFound
If the FutureChain is initialized with a root symbol for which

This comment has been minimized.

@ssanderson

ssanderson Jul 10, 2015

Member

Copywriting nit: I think I have a slight preference for "Raised when " rather than "If <condition" for the opening of the sentence.

👍 for the completeness of this docstring!

This comment has been minimized.

@yankees714

yankees714 Jul 10, 2015

Contributor

Agreed, that's a bit nicer.

self._maybe_update_current_chain()
def __repr__(self):
""" NOTE: This is really only a "pseudo" repr, in that the

This comment has been minimized.

@ssanderson

ssanderson Jul 10, 2015

Member

This probably should be a comment, not a docstring.
FWIW, the property that eval(repr(obj)) == obj isn't so ubiquitous that it's a big deal for it not to hold here.

Returns
-------
[Future]

This comment has been minimized.

@ssanderson

ssanderson Jul 10, 2015

Member

I'm a little confused by the brackets here. Is this indicating "List of Future"?

This comment has been minimized.

@yankees714

yankees714 Jul 10, 2015

Contributor

Yeah, I'll just change the type here to list and indicate in the description that it's a list of futures.

@yankees714 yankees714 force-pushed the futures-api branch 2 times, most recently from acd4e8b to 0730680 Jul 10, 2015

@jfkirk

This comment has been minimized.

Contributor

jfkirk commented Jul 10, 2015

Great work! 👍 Once the Jenkins gods have been appeased.

@yankees714 yankees714 force-pushed the futures-api branch 3 times, most recently from 1eb7d7a to d08f4f9 Jul 10, 2015

@yankees714 yankees714 force-pushed the futures-api branch from d08f4f9 to 2ab9f8a Jul 13, 2015

@yankees714 yankees714 merged commit 2ab9f8a into master Jul 13, 2015

3 checks passed

Scrutinizer 20 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@yankees714 yankees714 deleted the futures-api branch Jul 13, 2015

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