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 fixed percent slippage model #2047

Merged
merged 17 commits into from Dec 14, 2017

Conversation

Projects
None yet
4 participants
@vikram-narayan
Contributor

vikram-narayan commented Dec 7, 2017

add FixedBasisPointsSlippage which models slippage as a fixed percentage of fill price

@vikram-narayan vikram-narayan requested a review from ssanderson Dec 7, 2017

bps : int, optional
basis points to apply
"""
def __init__(self, bps=5):

This comment has been minimized.

@ssanderson

ssanderson Dec 7, 2017

Member

I would call this basis_points to match the __repr__.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 8, 2017

Contributor

fixed

@dmichalowicz dmichalowicz self-assigned this Dec 7, 2017

@@ -502,3 +502,30 @@ def get_simulated_impact(self,
def get_txn_volume(self, data, order):
volume = data.current(order.asset, 'volume')
return volume * self.volume_limit
class FixedBasisPointsSlippage(SlippageModel):

This comment has been minimized.

@ssanderson

ssanderson Dec 7, 2017

Member

We should add tests for this to tests/finance/test_slippage.py.

This comment has been minimized.

@ssanderson

ssanderson Dec 7, 2017

Member

VolumeShareSlippageTestCase looks like it would be a reasonable model to look at for adding these.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 8, 2017

Contributor

added a test for this slippage model (may change the test if we change the model)

def __repr__(self):
return '{class_name}(basis_points={bps})'.format(
class_name=self.__class__.__name__, spread=self.bps,

This comment has been minimized.

@dmichalowicz

dmichalowicz Dec 7, 2017

Contributor

I think you meant bps=self.bps here.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 8, 2017

Contributor

whoops, fixed

price = data.current(order.asset, "close")
return (
price + price * (self.bps / 10000.0 * order.direction),

This comment has been minimized.

@ssanderson

ssanderson Dec 7, 2017

Member

Small perf note: there's no reason to do this division on every order we process; we can just do this once at construction time.

This comment has been minimized.

@ssanderson

ssanderson Dec 7, 2017

Member

I'm wondering if we should be capping the percentage of the bar that we fill here. I know that part of the goal of adding this model is to give a reasonable estimate without simulating a huge number of transactions, but it seems pretty inaccurate to fill a 1000 share order in a minute where only 10 shares transacted.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 8, 2017

Contributor
  • added self.percentage to avoid doing division on every order
  • will change the slippage model depending on what we decide about that
class FixedBasisPointsSlippage(SlippageModel):
"""
Model slippage as a fixed percentage of fill price.

This comment has been minimized.

@ssanderson

ssanderson Dec 7, 2017

Member

This should probably also mention that it executes the full order immediately.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 8, 2017

Contributor

updated the docstring

@coveralls

This comment has been minimized.

coveralls commented Dec 7, 2017

Coverage Status

Coverage decreased (-0.03%) to 87.347% when pulling 48a7f65 on fixed_bps_slippage into 42b3200 on master.

@coveralls

This comment has been minimized.

coveralls commented Dec 8, 2017

Coverage Status

Coverage increased (+0.002%) to 87.374% when pulling d51d7f5 on fixed_bps_slippage into 42b3200 on master.

@vikram-narayan

This comment has been minimized.

Contributor

vikram-narayan commented Dec 11, 2017

@dmichalowicz @ssanderson added 10% volume limit, mind taking a quick look?

@coveralls

This comment has been minimized.

coveralls commented Dec 11, 2017

Coverage Status

Coverage increased (+0.005%) to 87.377% when pulling 4c94cc6 on fixed_bps_slippage into 42b3200 on master.

Parameters
----------
basis_points : int, optional

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

Is there a reason this couldn't be a float?

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

true, updated docstring for this to be a float, and changed the order amount to be castto an int

Parameters
----------
basis_points : int, optional
basis points to apply

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

Can we be clearer here about what "to apply" means? e.g.:

Number of basis points of slippage to apply on each execution. 

Orders to buy will be filled at `price + (price * basis_points * 0.0001)`.  
Orders to sell will be filled at `price -  (price * basis_points * 0.0001)`.
``

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

We should document volume_limit as well now.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

updated docstring

def process_order(self, data, order):
volume = data.current(order.asset, "volume")
max_volume = self.volume_limit * volume

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

Does this need to be truncated to an integer?

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

truncated this

WithSimParams,
ZiplineTestCase):
START_DATE = pd.Timestamp('2006-01-05 14:31', tz='utc')

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

I think many of our fixtures assume that START_DATE and END_DATE are set to midnight UTC of a given date. Do we have other fixtures that use intraday minutes as START_DATE and END_DATE?

class FixedBasisPointsSlippageTestCase(WithCreateBarData,
WithSimParams,

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

Why do we need WithSimParams here? It doesn't look like anything in this test depends on sim params being available.

def test_fixed_bps_slippage(self):
slippage_model = FixedBasisPointsSlippage()

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

We should test that this actually respects the basis points value that it's given. One way to do that would be to use a parameterized test, either with parameter_space or with nose.parameterized.

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

This would also be a reasonable way to test different amounts.

This comment has been minimized.

@dmichalowicz

dmichalowicz Dec 11, 2017

Contributor

In that vein, probably worth having a test for an amount less than the volume limit.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

added these tests

@classmethod
def init_class_fixtures(cls):
super(FixedBasisPointsSlippageTestCase, cls).init_class_fixtures()
cls.ASSET133 = cls.env.asset_finder.retrieve_asset(133)

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

Just use self.asset_finder here. TradingEnvironment is pretty vestigial, and any tests that are still using are just doing so by historical accident.

This comment has been minimized.

@vikram-narayan
)
@classproperty
def CREATE_BARDATA_DATA_FREQUENCY(cls):

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

Since we're not actually using sim params in the tests here, I would probably just set this attribute directly at class scope and remove the dependency on sim_params entirely.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

removed SimParams

open_orders = [
Order(
dt=datetime.datetime(2006, 1, 5, 14, 30, tzinfo=pytz.utc),
amount=100,

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

We should probably have a test case for negative amounts to ensure that we're adjusting in the correct direction on shorts.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

added this test

def process_order(self, data, order):
volume = data.current(order.asset, "volume")
max_volume = self.volume_limit * volume

This comment has been minimized.

@ssanderson

ssanderson Dec 11, 2017

Member

I think this also needs to account for the fact that we might have already filled a different order for this asset. VolumeShareSlippage uses self.volume_for_bar (which is maintained by the base class) to track this.

This comment has been minimized.

@vikram-narayan

vikram-narayan Dec 12, 2017

Contributor

updated to subtract volume_for_bar from order amount

@coveralls

This comment has been minimized.

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.005%) to 87.377% when pulling 3e89831 on fixed_bps_slippage into 42b3200 on master.

@coveralls

This comment has been minimized.

coveralls commented Dec 12, 2017

Coverage Status

Coverage increased (+0.005%) to 87.377% when pulling 3e89831 on fixed_bps_slippage into 42b3200 on master.

@vikram-narayan

This comment has been minimized.

Contributor

vikram-narayan commented Dec 12, 2017

added more tests, updated docstring, and used self.volume_for_bar to account for previous orders

@coveralls

This comment has been minimized.

coveralls commented Dec 13, 2017

Coverage Status

Coverage increased (+0.2%) to 87.542% when pulling bbf76b9 on fixed_bps_slippage into 42b3200 on master.

Fixed bps slippage notes (#2050)
MAINT: PR Notes
@coveralls

This comment has been minimized.

coveralls commented Dec 13, 2017

Coverage Status

Coverage increased (+0.2%) to 87.545% when pulling 1dd8fc0 on fixed_bps_slippage into 42b3200 on master.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Dec 14, 2017

LGTM

@vikram-narayan vikram-narayan merged commit fdfce9b into master Dec 14, 2017

2 checks passed

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

@vikram-narayan vikram-narayan deleted the fixed_bps_slippage branch Dec 14, 2017

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