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

Create MinLeverage control #2064

Merged
merged 1 commit into from Dec 29, 2017

Conversation

Projects
None yet
3 participants
@srklonaris
Contributor

srklonaris commented Dec 28, 2017

Similar to MaxLeverage create a MinLeverage control.

MinLeverage takes a float as well as a deadline date. MinLeverage validations will only happen after the algorithm has reached the deadline date. This allows algorithms enough time to reach their min leverage because it may take time for them to have a non zero leverage.

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2017

Coverage Status

Coverage decreased (-0.003%) to 87.559% when pulling 360670e on min-leverage into 7c6920c on master.

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2017

Coverage Status

Coverage decreased (-0.003%) to 87.559% when pulling 360670e on min-leverage into 7c6920c on master.

@srklonaris srklonaris force-pushed the min-leverage branch from 360670e to 070027d Dec 28, 2017

@ssanderson

@srklonaris took a first pass here

def __init__(self, min_leverage, deadline):
"""
max_leverage is the gross leverage in decimal form. For example,

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

Docstring here is still copied from MaxLeverage. We've also standardized on numpy-style docstrings in zipline since MaxLeverage was written. I don't think we need to go back and change existing code, but for new docstrings we should still use the style we use elsewhere in Zipline.

self.min_leverage = min_leverage
self.deadline = deadline
if min_leverage is None:

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

You might want to look at using the decorators in zipline.utils.input_validation to implement these checks. I think expect_types(min_leverage=(int, float), deadline=datetime) and expect_strictly_bounded(min_leverage=(0, None)) would capture the behavior we want here. If you do end up using those functions, you probably want to pass __funcname='MinLeverage' so that the generated error messages reference MinLeverage instead of __init__.

@@ -485,6 +485,11 @@ def initialize(self, max_leverage=None):
self.set_max_leverage(max_leverage=max_leverage)
class SetMinLeverageAlgorithm(TradingAlgorithm):
def initialize(self, min_leverage=None, deadline=None):

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

Should we just make these required if it's invalid to pass None for these values to set_min_leverage?

min_leverage : float
The minimum leverage for the algorithm. If not provided there will
be no minimum.
deadline : datetime

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

This might be more nicely expressed as a timedelta from the start of the algorithm instead of a datetime.

Parameters
----------
min_leverage : float
The minimum leverage for the algorithm. If not provided there will

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

Is the second sentence here actually true? If not, should we just remove it?

@@ -2234,6 +2235,23 @@ def set_max_leverage(self, max_leverage):
control = MaxLeverage(max_leverage)
self.register_account_control(control)
@api_method
def set_min_leverage(self, min_leverage, timedelta):

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

I would still call this deadline. timedelta is shadowing the name of datetime.timedelta, which is usually frowned upon.

This comment has been minimized.

@srklonaris

srklonaris Dec 28, 2017

Contributor

What about offset, since deadline is what is computed and passed into MinLeverage

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

I don't think offset would be a very clear name for this parameter for a user of this function. My immediate question would be "offset from what"? Maybe grace_period?

# Algo date is not past deadline so algorithm succeeds
offset = pd.Timedelta('10 days')
# Set min leverage to 0 so buying one share fails.

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

This comment doesn't match the test case.

self.check_algo_fails(algo, handle_data)
self.assertEqual(
algo.recorded_vars['latest_time'],
pd.Timestamp('2006-01-04 21:00:00', tz='UTC'),

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

Can we also test that this fails at 01-05 if we increase the offset delta to 2 days? (I might just break out those two cases into separate test methods instead of having all the cases at once).

offset = pd.Timedelta('10 days')
# Set min leverage to 0 so buying one share fails.
def handle_data(algo, data):

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

All the handle_data functions we define in this method are functionally the same. Is there a reason we're defining a separate one each time?

algo = SetMinLeverageAlgorithm(0, offset, sim_params=self.sim_params,
env=self.env)
self.check_algo_succeeds(algo, handle_data)

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

I think we need a test case where we set a minimum leverage amount and the algorithm has a leverage above that limit after the deadline and doesn't crash. Bonus points if the algorithm then goes below the leverage limit and crashes.

This comment has been minimized.

@ssanderson

ssanderson Dec 28, 2017

Member

This might be easier to implement using order_target_percent to hit fixed leverage amounts instead of ordering fixed numbers of shares.

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2017

Coverage Status

Coverage decreased (-0.003%) to 87.56% when pulling 070027d on min-leverage into 7c6920c on master.

@srklonaris srklonaris force-pushed the min-leverage branch 2 times, most recently from 716a4ff to e4955de Dec 28, 2017

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.01%) to 87.576% when pulling e4955de on min-leverage into 7c6920c on master.

@coveralls

This comment has been minimized.

coveralls commented Dec 28, 2017

Coverage Status

Coverage increased (+0.01%) to 87.576% when pulling e4955de on min-leverage into 7c6920c on master.

@srklonaris srklonaris force-pushed the min-leverage branch 2 times, most recently from a903bde to 4ba07c7 Dec 29, 2017

@coveralls

This comment has been minimized.

coveralls commented Dec 29, 2017

Coverage Status

Coverage increased (+0.01%) to 87.576% when pulling 4ba07c7 on min-leverage into 7c6920c on master.

@ssanderson

@srklonaris had a few small notes, but generally this looks good!

algo.order_target_percent(algo.sid(self.sidint), .5)
algo.record(latest_time=algo.get_datetime())
# Set min leverage to 1.

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

I don't think this comment matches the test case?

env=self.env)
self.check_algo_succeeds(algo, handle_data)
# The algorithm will fail because it doesn't reach a min leverage of 1

This comment has been minimized.

@ssanderson
# The algorithm will succeed because it doesn't run for more
# than 10 days.
offset = pd.Timedelta('10 days')
algo = SetMinLeverageAlgorithm(0, offset, sim_params=self.sim_params,

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

I think this should be a 1?

pd.Timestamp('2006-01-04 21:00:00', tz='UTC'),
)
# Increase the offset to 2 days, and the algorithm fails a day later

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

This is nicely clear to follow as a reader 👍 .

min_leverage : float
The minimum leverage for the algorithm.
grace_period : pd.Timedelta
The offset from the start date used to enforce a minimum leverage

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

Missing period here.

@expect_types(
__funcname='MinLeverage',
min_leverage=(int, float),
deadline=pd.datetime

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

pd.datetime is just an alias of the standard library datetime.datetime (presumably b/c something in the top level pandas module does from datetime import datetime). We should use the standard library location here.

)
@expect_bounded(__funcname='MinLeverage', min_leverage=(0, None))
def __init__(self, min_leverage, deadline):
"""

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

We generally don't put docstrings on __init__ methods; we use the class docstring documenting the parameters to a class and describing what it's for.

class MinLeverage(AccountControl):
"""

This comment has been minimized.

@ssanderson

ssanderson Dec 29, 2017

Member

This should probably also be a numpy-style docstring.

@srklonaris srklonaris force-pushed the min-leverage branch from 4ba07c7 to fa711f8 Dec 29, 2017

@coveralls

This comment has been minimized.

coveralls commented Dec 29, 2017

Coverage Status

Coverage increased (+0.01%) to 87.577% when pulling fa711f8 on min-leverage into 7c6920c on master.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Dec 29, 2017

LGTM

@srklonaris srklonaris merged commit 5978f68 into master Dec 29, 2017

2 checks passed

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

@srklonaris srklonaris deleted the min-leverage branch Dec 29, 2017

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