Skip to content
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: Add trading controls to zipline API. #329

Merged
merged 2 commits into from May 12, 2014
Merged

ENH: Add trading controls to zipline API. #329

merged 2 commits into from May 12, 2014

Conversation

@ssanderson
Copy link
Contributor

@ssanderson ssanderson commented May 3, 2014

Adds four new methods to the Zipline API that can be used as circuit-breakers
to interrupt the execution of an algorithm. The API methods are:

set_max_position_size
set_max_order_size
set_max_order_count
set_long_only

Internally, these methods are implemented by each registering a TradingControl
callback object with the TradingAlgorithm. During
TradingAlgorithm.__validate_order_params (and thus before any side-effects of
the order call occur), each callback's validate method is called with
information about the order to be placed and the algorithm's current state,
raising an exception if the callback detects that an error condition has been breached.

@@ -13,16 +13,23 @@
# See the License for the specific language governing permissions and
# limitations under the License.

from unittest import TestCase
from contextlib2 import ExitStack as Noop

This comment has been minimized.

@twiecki

twiecki May 7, 2014
Contributor

We're not using this anywhere else, right? Seems like this needs to be added to .travis.yaml and other places as a dependency. Unfortunately this doesn't exist in conda which could cause problems for our current installation. Is there a way around it (haven't looked below yet)?

This comment has been minimized.

@ssanderson

ssanderson May 7, 2014
Author Contributor

This is just being used as a null contextmanager to make a complicated nested setup a bit more readable. We could reimplement Noop as just:

class Noop(object):
    @staticmethod
    def __enter__():
        pass
    @staticmethod
    def __exit__():
        pass

This comment has been minimized.

@twiecki

twiecki May 7, 2014
Contributor

OK, yeah. I think that'd be easier in this case rather than adding a dependency.

This comment has been minimized.

@ssanderson

ssanderson May 7, 2014
Author Contributor

Or better yet:

from contextlib import contextmanager
@contextmanager
def noop():
    yield

This comment has been minimized.

@ssanderson

ssanderson May 7, 2014
Author Contributor

Updated this to use a home-grown noop contextmanager.

self.trading_controls.append(control)

@api_method
def set_max_position_size(self, sid, max_shares=None, max_notional=None):

This comment has been minimized.

@twiecki

twiecki May 7, 2014
Contributor

Just chatted with @jbredeche and it seems the spec changed to not require a sid to be passed? This applies to the functions below as well.

This comment has been minimized.

@ssanderson

ssanderson May 7, 2014
Author Contributor

Updated this to default to sid=None and always do a check in validate if self.sid is None.

@twiecki
Copy link
Contributor

@twiecki twiecki commented May 7, 2014

One question I had is how this would work if we decided to add default limits. If we're sure we never want to do this it doesn't matter, but I think using a list to store the limits would make changing a default-limit difficult. If the limits are global we could instead have a dict that stores all possible limits and a call to the set_ method would update them.

@ssanderson
Copy link
Contributor Author

@ssanderson ssanderson commented May 7, 2014

I think the right way to manage default controls (or in general being able to replace a control with another control) would be to override TradingControl.__eq__ to do an equality check on the dictionary of instance arguments that we store for use in repr. Then register_trading_control would look like:

def register_trading_control(self, control):
    self.trading_controls = filter(lambda x: x != control).append(control)
@ssanderson
Copy link
Contributor Author

@ssanderson ssanderson commented May 9, 2014

@twiecki @ehebert I think this is good to ship unless there are further comments?

@ehebert
Copy link
Contributor

@ehebert ehebert commented May 9, 2014

I think @twiecki spent some more time looking at it than me, it looks ok to me.

Though, for debug-ability/stepping through with a debugger I'd suggest dict instead of list for the trading_controls member. So that trading_controls['max_count'] could be referred to instead of finding it in the list. But that's not show stopping.

An ignore-able request is to do the alphasort of imports in another commit, so that it is clearer what is added with this patch.

@twiecki
Copy link
Contributor

@twiecki twiecki commented May 9, 2014

Yeah, the dict would be my only recommendation.

@twiecki
Copy link
Contributor

@twiecki twiecki commented May 9, 2014

But looks good otherwise! 👍

@twiecki
Copy link
Contributor

@twiecki twiecki commented May 10, 2014

Also, can you add a feature description to the release notes like here c444587?

@ssanderson
Copy link
Contributor Author

@ssanderson ssanderson commented May 12, 2014

I think storing the trading_controls member as dict might be awkward because you can have multiple trading controls of each type (e.g. separate max_shares limits for 3 different sids), so there isn't a natural key unless the data structure was mapping str -> list[control]. I'm inclined to keep the representation as is for now. I don't think debugability will be too much of an issue since these controls all have nice __repr__s, and the expectation for the time being is that an algo is going to register on the order of 4-5 of these.

@ehebert
Copy link
Contributor

@ehebert ehebert commented May 12, 2014

Sounds good to leave it as a list for now, then.
Thanks for weighing it against the alternative.

ssanderson added 2 commits May 3, 2014
Adds four new methods to the Zipline API that can be used as circuit-breakers
to interrupt the execution of an algorithm.  The API methods are:

`set_max_position_size`
`set_max_order_size`
`set_max_order_count`
`set_long_only`

Internally, these methods are implemented by each registering a TradingControl
callback object with the TradingAlgorithm.  During
TradingAlgorithm.__validate_order_params (and thus before any side-effects of
the order call occur), each callback's `validate` method is called with
information about the order to be placed and the algorithm's current state,
raising an exception if the callback detects that an error condition has been breached.
@ssanderson ssanderson merged commit 2976a0d into master May 12, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@ssanderson ssanderson deleted the trading_controls branch May 12, 2014
@ssanderson
Copy link
Contributor Author

@ssanderson ssanderson commented May 12, 2014

Added release notes and closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.