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

Add slippage and commission models for futures #1748

Merged
merged 1 commit into from Apr 25, 2017

Conversation

Projects
None yet
5 participants
@dmichalowicz
Contributor

dmichalowicz commented Apr 11, 2017

This PR adds the actual slippage and commissions models for futures, taking advantage of the infrastructure built here.

In terms of the API, the set_slippage and set_commission functions now accept an equities argument and a futures argument for setting two different models according to the type of asset being traded. Furthermore, the SlippageModel and CommissionModel classes have been broken out into Equity and Future subclasses. This is to help enforce that the models passed to these new parameters are of the correct type. For example, the equities parameter for set_slippage must inherit from EquitySlippageModel, while the futures parameter must inherit from FutureSlippageModel.

For futures commissions, we use PerContract by default, which is essentially the equivalent of PerShare for the number of contracts traded. Unlike PerShare however, the commission for each different root symbol can be toggled by passing a dictionary mapping root symbol to its respective commission cost.

root_symbol = order.asset.root_symbol
exchange_fee = self.exchange_fee.get(
root_symbol, FUTURE_EXCHANGE_FEES_BY_SYMBOL[root_symbol],
)

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 12, 2017

Contributor

The logic here that differs according to the type of exchange_fee and cost_per_contract seems well suited for something like multiple dispatch, but I'm not sure how that would work if these values aren't parameters to the method.

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

I would do this by putting the float in a dummy "mapping" once in the constructor and then always programming against a mapping interface:

class DummyMapping(object):
    def __init__(self, value):
        self._value = value
    def __getitem__(self, key):
        return self._value

class FallbackMap(object):
    def __init__(self, 

In your constructor:

if isinstance(exchange_fee, float):
    self._exchange_fees = DummyMapping(exchange_fee)
else:
    self._exchange_fees = toolz.merge(DEFAULT_FUTURE_EXCHANGE_FEES, exchange_fees)

Then all your use sites can just be fee = self._exchange_fees[root_symbol].

The general observation here is that if you're doing runtime dispatch in a method called repeatedly, often a good solution is to adapt your possible inputs to a common interface at construction time and then program against that interface.

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

Thanks for the suggestion, this is definitely an improved way of doing it.

@dmichalowicz dmichalowicz changed the title from WIP: Add slippage and commission models for futures to Add slippage and commission models for futures Apr 13, 2017

@dmichalowicz dmichalowicz force-pushed the slippage-futures-api branch from 633cb60 to 89d0d68 Apr 17, 2017

@dmichalowicz dmichalowicz requested a review from ssanderson Apr 17, 2017

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Apr 17, 2017

@ssanderson This is ready for a first look. cc @jbredeche in case you were interested.

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.1%) to 87.664% when pulling 53336dd on slippage-futures-api into 4c334c6 on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.1%) to 87.664% when pulling 875cd8c on slippage-futures-api into 4c334c6 on master.

def _get_window_data(self, data, asset, window_length):
"""
Internal utility method to return the trailing 20-day mean volume and

This comment has been minimized.

@prsutherland

prsutherland Apr 21, 2017

Member

We can probably remove the "20-day" in the doc now that it is parameterized and add it to the doc's parameter list.

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

good catch

try:
values = self._window_data_cache.get(asset, data.current_session)
except KeyError:
# Add a day because we wan 'window_length' complete days.

This comment has been minimized.

@prsutherland

prsutherland Apr 21, 2017

Member

Small typo in "we want".

eta = ROOT_SYMBOL_TO_ETA[order.asset.root_symbol]
mean_volume, volatility = self._get_window_data(data, order.asset, 20)
if mean_volume == 0:

This comment has been minimized.

@prsutherland

prsutherland Apr 21, 2017

Member

You probably want to check volatility too. If you have less than 20 days but more than 0, the volatility will be NaN but the mean volume may not be 0

impacted_price = \
price + math.copysign(simulated_impact, order.direction)
else:
txn_volume = int(

This comment has been minimized.

@prsutherland

prsutherland Apr 21, 2017

Member

I'm wondering if we should refactor this method so that it can be shared between VolatilityVolumeShare and MarketImpact and this else block can be a call to get simulated_impact because that appears to be the only place this logic differs.

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

Good idea, will do

@ssanderson

@dmichalowicz finished a first pass on this.

# Note that the exchange fee is a one-time cost that is only applied to
# the first fill of an order.
#
# The commission on the first fill is (230 * 0.01) + 0.3 = 2.6

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Where is the first number (e.g. 230 and 170) coming from in these calculations?

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Ah, looks like it's the amount of the order/transaction generated by verify_per_unit_commissions. That's not at all clear here as a reader. I'd strongly recommend making the order and fill amounts parameters at these call sites. Not having the inputs visible obscures the relationship between input and output that's under test here.

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

Agreed, will change.

)
# Test per trade model with custom costs per future symbol.
model = PerFutureTrade(cost_per_trade={'CL': 5, 'FV': 10})

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Using strings as the keys for this configuration feels like it's going to bite us in the future (no pun intended). Is there a reason to prefer strings over continuous futures here?

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

@jmccorriston I vaguely recall talking about this in our design discussions the other day. I thought we had come to the conclusion that we wanted to use continuous futures here?

self.verify_per_trade_commissions(model, expected_commission=10, sid=1)
# Test per trade model for futures.
model = PerFutureTrade(cost_per_trade=10)

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

It seems odd that the parameter name here is cost_per_trade when the parameter to PerTrade is just cost. Given that the name of the class has PerTrade already, putting it in the parameter name as well seems redundant.

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

Will change to cost

# Minimum is met by the first trade.
self.verify_per_unit_commissions(
PerContract(
cost_per_contract=.01, exchange_fee=0.3, min_trade_cost=1,

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Same note as above re: redundancy in putting the unit in the name here.

@@ -249,6 +440,29 @@ def test_per_dollar(self):
self.verify_capital_used(results, [-1010, -1010, -1010])
def test_incorrectly_set_futures_model(self):
with self.assertRaises(UnsupportedCommissionModel):

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

It might be worth carving out a separate error class for the case of "You passed a futures model but we expected an equities model" and vice-versa.

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

(As opposed to "You didn't even give us a commission model", which is what UnsupportedCommissionModel is used for now.)

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

Good idea

if mean_volume == 0:
# If this is the first day the contract exists and there is no
# volume history, default to an impact of 10 bps.
simulated_impact = price * (10.0 / 10000)

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Can we make this a symbolic constant? (e.g. NO_DATA_VOLATILITY_SLIPPAGE_IMPACT)

This comment has been minimized.

@dmichalowicz
return values['volume'], values['close']
class VolatilityVolumeShare(WithWindowData, FutureSlippageModel):

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

This needs docs.

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

will add

# volume history, default to an impact of 10 bps.
simulated_impact = price * (10.0 / 10000)
impacted_price = \

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

This expression is duplicated twice. Can we just compute simulated_impact in the branches here and then compute impacted_price once?

This comment has been minimized.

@dmichalowicz
eta = ROOT_SYMBOL_TO_ETA[order.asset.root_symbol]
mean_volume, volatility = self._get_window_data(data, order.asset, 20)
if mean_volume == 0:

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

I think this branch is broken right now (we never compute bind txn_volume in this branch).

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

I added a test for this case (test_calculate_impact_without_history)

)
psi = txn_volume / mean_volume
market_impact = eta * volatility * math.sqrt(psi)

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Why is this sqrt?

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

This is just according to the model that @marketneutral prescribed.

@dmichalowicz dmichalowicz force-pushed the slippage-futures-api branch from a1ad7a3 to cefc792 Apr 25, 2017

allowed_asset_types = (Future,)
def __init__(self, volume_limit, eta=ROOT_SYMBOL_TO_ETA):
super(VolatilityVolumeShare, self).__init__()

This comment has been minimized.

@jbredeche

jbredeche Apr 25, 2017

Member

might be useful to have documentation on this method explaining the types of the parameters, especially eta - took me a while to figure out what the allowed types were for it.

}
self._window_data_cache.set(asset, values, data.current_session)
# from nose.tools import set_trace; set_trace()

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 25, 2017

Contributor

oops will remove

@dmichalowicz dmichalowicz force-pushed the slippage-futures-api branch from cefc792 to dd21346 Apr 25, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 25, 2017

Coverage Status

Coverage increased (+0.5%) to 87.703% when pulling dd21346 on slippage-futures-api into 0da8a59 on master.

@dmichalowicz

This comment has been minimized.

Contributor

dmichalowicz commented Apr 25, 2017

Got the OK from @jbredeche to merge.

@dmichalowicz dmichalowicz merged commit 3af85a6 into master Apr 25, 2017

2 checks passed

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

@dmichalowicz dmichalowicz deleted the slippage-futures-api branch Apr 25, 2017

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