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

DEV: Re-implement commission models to return correct results in the case of multiple fills #1213

Merged
merged 1 commit into from May 24, 2016

Conversation

Projects
None yet
3 participants
@jbredeche
Member

jbredeche commented May 19, 2016

No description provided.

@jbredeche

This comment has been minimized.

Member

jbredeche commented May 19, 2016

(still have a few test failures, working on it)

@@ -56,7 +56,7 @@
OrderInBeforeTradingStart)
from zipline.finance.trading import TradingEnvironment
from zipline.finance.blotter import Blotter
from zipline.finance.commission import PerShare, PerTrade, PerDollar
from zipline.finance.commission import PerShare, CommissionModelBase

This comment has been minimized.

@ssanderson

ssanderson May 19, 2016

Member

Most of our other base classes leave out Base in the name (e.g. PipelineLoader, TradingControl, ExecutionStyle).

This comment has been minimized.

@jbredeche

jbredeche May 20, 2016

Member

cool, changed

Returns
-------
float: The additional commission we should attribute to this order.

This comment has been minimized.

@ssanderson

ssanderson May 19, 2016

Member

Worth noting that this is a dollar amount?

This comment has been minimized.

@jbredeche

jbredeche May 20, 2016

Member

yup, done

self.min_trade_cost = None if min_trade_cost is None\
else float(min_trade_cost)
self.cost_per_share = float(cost)
self.min_trade_cost = None

This comment has been minimized.

@ssanderson

ssanderson May 19, 2016

Member

Why not just self.min_trade_cost = min_trade_cost? Not sure what the use-case is for passing a non-float. (This also means we're actually accepting things like strings here...)

This comment has been minimized.

@jbredeche

jbredeche May 20, 2016

Member

Agreed.

"""
cost_per_share = transaction.price * self.cost
return cost_per_share, abs(transaction.amount) * cost_per_share
cost_per_share = transaction.price * self.cost_per_dollar

This comment has been minimized.

@ssanderson

ssanderson May 19, 2016

Member

Curious: does the commission get calculated `before or after slippage has been applied?

This comment has been minimized.

@jbredeche

jbredeche May 20, 2016

Member

Afterwards. Slippage is applied when we generate the transaction.

first_txn = all_txns[0]
# all_orders are all the incremental versions of the
# orders as each new fill comes in.
all_orders = [order for orders in

This comment has been minimized.

@ssanderson

This comment has been minimized.

@ssanderson

ssanderson May 19, 2016

Member

I think this is just toolz.concat(results['orders']):

(Pdb++) all_orders == list(toolz.concat(results['orders']))
True

(toolz.concat is just a more-clearly-named itertools.chain.from_iterable).

This comment has been minimized.

@jbredeche

jbredeche May 20, 2016

Member

cool, thanks

@jbredeche jbredeche force-pushed the youre-charging-me-what branch 2 times, most recently from 9faa9c5 to 2d4ab89 May 20, 2016

@coveralls

This comment has been minimized.

coveralls commented May 20, 2016

Coverage Status

Coverage decreased (-0.01%) to 81.674% when pulling 9faa9c5 on youre-charging-me-what into 96ec1fd on master.

@jbredeche jbredeche force-pushed the youre-charging-me-what branch from 2d4ab89 to 46e6353 May 20, 2016

@coveralls

This comment has been minimized.

coveralls commented May 20, 2016

Coverage Status

Coverage decreased (-0.01%) to 81.671% when pulling 46e6353 on youre-charging-me-what into 96ec1fd on master.

@coveralls

This comment has been minimized.

coveralls commented May 23, 2016

Coverage Status

Coverage increased (+0.02%) to 81.702% when pulling d745f82 on youre-charging-me-what into 96ec1fd on master.

@jbredeche jbredeche force-pushed the youre-charging-me-what branch from 7656113 to 39bf1db May 24, 2016

@jbredeche jbredeche merged commit 1630dc6 into master May 24, 2016

2 checks passed

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

@jbredeche jbredeche deleted the youre-charging-me-what branch May 24, 2016

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