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

Order batch #1596

Merged
merged 12 commits into from Dec 22, 2016

Conversation

Projects
None yet
3 participants
@richafrank
Member

richafrank commented Nov 18, 2016

Adds order_batch to the blotter and factors out ordering helper methods, so one can generate the orders and give them to order_batch.

@coveralls

This comment has been minimized.

coveralls commented Nov 18, 2016

Coverage Status

Coverage increased (+0.01%) to 86.994% when pulling 446fe72 on order_batch into 515db18 on master.

asset_24 = blotter.asset_finder.retrieve_asset(24)

This comment has been minimized.

@ssanderson

ssanderson Nov 21, 2016

Member

Not super important, but we might make this diff less noisy by just replacing this line with asset_24 = self.asset_24 instead of changing every usage below.

@@ -91,7 +91,7 @@ def order(self, sid, amount, style, order_id=None):
"""
if amount == 0:
# Don't bother placing orders for 0 shares.
return

This comment has been minimized.

@ssanderson
@@ -114,6 +114,9 @@ def order(self, sid, amount, style, order_id=None):
return order.id
def order_batch(self, orders):

This comment has been minimized.

@ssanderson

ssanderson Nov 21, 2016

Member

Do we want to document this in some way? (Or at least comment it so we don't remove it?)

This comment has been minimized.

@ssanderson

ssanderson Nov 21, 2016

Member

orders here makes me think this is an iterable of Order objects. Not sure what's a bettter name though, since order_args makes me think it's a single tuple of arguments, and order_argses just sounds like hobbit-speak.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Nov 21, 2016

@richafrank finished this round. No major issues. Had a couple questions about naming/docs for order_batch and a style note for some of the test refactoring.

@richafrank richafrank force-pushed the order_batch branch from 446fe72 to e512ef8 Nov 23, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 23, 2016

Coverage Status

Coverage increased (+0.01%) to 87.111% when pulling e512ef8 on order_batch into 4174a09 on master.

@coveralls

This comment has been minimized.

coveralls commented Nov 28, 2016

Coverage Status

Coverage increased (+0.1%) to 87.238% when pulling a0955cf on order_batch into 4174a09 on master.

@richafrank richafrank force-pushed the order_batch branch from a0955cf to 87d3005 Nov 30, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2016

Coverage Status

Coverage increased (+0.02%) to 87.145% when pulling 87d3005 on order_batch into 0e3b290 on master.

@richafrank richafrank force-pushed the order_batch branch from 87d3005 to a59b678 Nov 30, 2016

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2016

Coverage Status

Coverage increased (+0.02%) to 87.146% when pulling a59b678 on order_batch into 9ac957f on master.

order_args = OrderedDict()
for asset, target in iteritems(weights):
if self._can_order_asset(asset):
amount = self._calculate_order_target_percent_amount(

This comment has been minimized.

@ssanderson

ssanderson Dec 1, 2016

Member

Should we continue here if amount is 0?

This comment has been minimized.

@richafrank

richafrank Dec 1, 2016

Member

I figured we shouldn't check the amount in multiple places, but what do you think? We check it currently in Blotter.order.

This comment has been minimized.

@ssanderson

ssanderson Dec 1, 2016

Member

I think there's a semantic difference between those cases though. The way this works right now, we're going to return a null entry for the size-zero order, whereas if we continue here, there won't be an entry at all.

This comment has been minimized.

@richafrank

richafrank Dec 1, 2016

Member

I'll filter out the null entries before returning them to the caller.

@@ -75,12 +75,29 @@ def set_date(self, dt):
self.current_dt = dt
def order(self, sid, amount, style, order_id=None):
"""Place an order.

This comment has been minimized.

@ssanderson

ssanderson Dec 1, 2016

Member

👍 📝

class TestBatchTargetPercentAlgorithm(TestTargetPercentAlgorithm):
def _order(self, asset, target):
return self.batch_order_target_percent({asset: target})

This comment has been minimized.

@ssanderson

ssanderson Dec 1, 2016

Member

Do we want a test for actually ordering more than one asset at a time?

This comment has been minimized.

@richafrank

richafrank Dec 1, 2016

Member

Yes, definitely - that's the test I still need to add.

@coveralls

This comment has been minimized.

coveralls commented Dec 1, 2016

Coverage Status

Coverage increased (+0.02%) to 87.153% when pulling 2878bb7 on order_batch into 9ac957f on master.

@richafrank richafrank force-pushed the order_batch branch from 2878bb7 to d50eee7 Dec 2, 2016

@coveralls

This comment has been minimized.

coveralls commented Dec 2, 2016

Coverage Status

Coverage increased (+0.02%) to 87.153% when pulling d50eee7 on order_batch into 4765136 on master.

@coveralls

This comment has been minimized.

coveralls commented Dec 9, 2016

Coverage Status

Coverage increased (+0.08%) to 87.217% when pulling 7d96340 on order_batch into 4765136 on master.

@richafrank richafrank force-pushed the order_batch branch from 7d96340 to 381029a Dec 20, 2016

@coveralls

This comment has been minimized.

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+0.02%) to 87.157% when pulling 381029a on order_batch into 0848a8a on master.

@richafrank richafrank force-pushed the order_batch branch from 872b6a9 to e674e4e Dec 21, 2016

@coveralls

This comment has been minimized.

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.02%) to 87.165% when pulling e674e4e on order_batch into 0848a8a on master.

@coveralls

This comment has been minimized.

coveralls commented Dec 21, 2016

Coverage Status

Coverage increased (+0.02%) to 87.165% when pulling e674e4e on order_batch into 0848a8a on master.

@richafrank richafrank merged commit 3350227 into master Dec 22, 2016

2 checks passed

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

@richafrank richafrank deleted the order_batch branch Dec 22, 2016

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