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

Assets everywhere ... and a futures fix. #1757

Merged
merged 9 commits into from Apr 24, 2017

Conversation

Projects
None yet
5 participants
@jbredeche
Member

jbredeche commented Apr 20, 2017

The original point of this branch was to fix a bug in how we calculate a position's cost basis for futures (it wasn't taking into account contract size).

This led me into a refactoring that I had been meaning to do for a while, in which all mentions of sid were removed in favor of asset (the sid properties were almost always Asset objects already).

@jbredeche jbredeche force-pushed the futures-commissions branch from 3181f5f to a3a1ff0 Apr 20, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.01%) to 87.544% when pulling a3a1ff0 on futures-commissions into 4c334c6 on master.

@@ -242,6 +247,7 @@ def __repr__(self):
__getitem__ = _deprecated_getitem_method(
'position', {
'sid',
'asset',

This comment has been minimized.

@jbredeche

jbredeche Apr 20, 2017

Member

now the user-facing Position object will have both sid and asset properties. is that OK?

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

I think leaving sid around for compat is fine, but we don't need to add a __getitem__ dispatch for the new field.

@jbredeche jbredeche force-pushed the futures-commissions branch 3 times, most recently from 2e79839 to 0cc81a5 Apr 20, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.01%) to 87.546% when pulling 0cc81a5 on futures-commissions into 4c334c6 on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.01%) to 87.546% when pulling 0cc81a5 on futures-commissions into 4c334c6 on master.

@jbredeche jbredeche force-pushed the futures-commissions branch from 0cc81a5 to aabb914 Apr 20, 2017

@jbredeche

This comment has been minimized.

Member

jbredeche commented Apr 20, 2017

cc @ssanderson , the refactoring we have been discussing.

@coveralls

This comment has been minimized.

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.009%) to 87.541% when pulling aabb914 on futures-commissions into 4c334c6 on master.

@jbredeche jbredeche force-pushed the futures-commissions branch from aabb914 to 1d0a2e3 Apr 21, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 21, 2017

Coverage Status

Coverage increased (+0.009%) to 87.541% when pulling 1d0a2e3 on futures-commissions into 4c334c6 on master.

@ssanderson ssanderson self-assigned this Apr 21, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 21, 2017

Coverage Status

Coverage increased (+0.008%) to 87.54% when pulling 735d6f4 on futures-commissions into 4c334c6 on master.

@ssanderson

@jbredeche finished a pass here. Generally 👍 on the premise.

There are a couple places where we're doing things like asset = finder.retrieve_asset(asset), which are either redundant or pretty confusing. If they're redundant, then I think we can drop most of the code around managing multiplier caches in the position/performance tracking code, (which I think means we can drop the asset finder as well).

@@ -167,14 +173,18 @@ def adjust_commission_cost_basis(self, sid, cost):
return
prev_cost = self.cost_basis * self.amount
new_cost = prev_cost + cost
if isinstance(asset, Future):

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

A bunch of our code ends up having to do this kind of dispatching because multiplier is only defined on Future. The change probably belongs in a separate PR, but what do you think about putting a multiplier property on Equity that just returns 1?

cc @ehebert @dmichalowicz @yankees714

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Also, this is the only intended functional change in this PR, right?

This comment has been minimized.

@jbredeche

jbredeche Apr 21, 2017

Member

Correct

This comment has been minimized.

@dmichalowicz

dmichalowicz Apr 22, 2017

Contributor

+1 on adding a multiplier to Equity.

This comment has been minimized.

@jbredeche

jbredeche Apr 22, 2017

Member

I'm +1 on that too, but not doing it in this PR.

This comment has been minimized.

@ehebert

ehebert Apr 22, 2017

Member

I am closer to 👎 than :1: on making the change to Equity to include a multiplier value.

  • There are places where we do dispatch because Equitiesand Futures behave differently.
    e.g. cases like period.PerformancePeriod.handle_execution where that value should be a no-op for Equitys. I believe in most cases we do want to dispatch on Asset type because we want to consider whether the contract contributes to exposure, should calculate a payout, etc.
  • multiplier is something that is inherit to the definition of a future contract; adding it to an Equity object would be clouding the modelling of an equity which may lead to regressions in treating Assets alike in cases where they should not be.

Needless to say, 'not for this PR', but have we considered modelling the portfolio by having an EquityPortfolio and a FuturePortfolio, where the overall Portfolio is compromised of the two?
This may allow us to model the unique behaviors of each more cleanly.

@@ -186,7 +196,7 @@ def to_dict(self):
Returns a dict object of the form:
"""
return {
'sid': self.sid,
'sid': self.asset,

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Do we still want this to be 'sid'?

This comment has been minimized.

@jbredeche

jbredeche Apr 22, 2017

Member

I think so. This is how the perf packets are constructed.

@@ -133,41 +136,42 @@ def __init__(self, asset_finder, data_frequency):
self.data_frequency = data_frequency
def _update_asset(self, sid):
def _update_asset(self, asset):

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

I think the point of this code was that we were keeping caches from integer -> multiplier because we didn't have the asset object. Since we now always have assets, can we just kill this method?

This comment has been minimized.

@jbredeche
# Collect the value multipliers from applicable sids
asset = self.asset_finder.retrieve_asset(sid)
# Collect the value multipliers from applicable assets
asset = self.asset_finder.retrieve_asset(asset)

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Do we still need to be retrieving the asset here? If so, the variable names here are somewhat misleading.

This comment has been minimized.

@jbredeche

jbredeche Apr 23, 2017

Member

nope. all gone.

def update_positions(self, positions):
# update positions in batch
self.positions.update(positions)
for sid, pos in iteritems(positions):
self._update_asset(sid)
for asset, pos in iteritems(positions):

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

I don't think we need this loop anymore.

This comment has been minimized.

@jbredeche

jbredeche Apr 23, 2017

Member

Entire method is gone now.

@@ -242,6 +247,7 @@ def __repr__(self):
__getitem__ = _deprecated_getitem_method(
'position', {
'sid',
'asset',

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

I think leaving sid around for compat is fine, but we don't need to add a __getitem__ dispatch for the new field.

# all our legacy order management code works with integer sids.
# this lets us convert those to assets when needed. ideally, we'd just
# revamp all the legacy code to work with assets.
self.asset_finder = asset_finder

This comment has been minimized.

@ssanderson
@@ -306,18 +303,19 @@ def process_splits(self, splits):
Parameters
----------
splits: list
A list of splits. Each split is a tuple of (sid, ratio).
A list of splits. Each split is a tuple of (asset, ratio), where
sid is an integer.
Returns
-------
None
"""
for split in splits:

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

This block might be a little easier to follow if we unpacked the split pairs in the loop.

for asset, ratio in splits:
    ...

This comment has been minimized.

@jbredeche

jbredeche Apr 22, 2017

Member

agreed, fixed

@@ -362,7 +362,7 @@ def record_order(self, order):
def handle_execution(self, txn):
self.cash_flow += self._calculate_execution_cash_flow(txn)
asset = self.asset_finder.retrieve_asset(txn.sid)
asset = self.asset_finder.retrieve_asset(txn.asset)

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

So we still need to retrieve the asset here?

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

(In general, do we still need an asset finder here?)

This comment has been minimized.

@jbredeche

jbredeche Apr 23, 2017

Member

no more assetfinder

except KeyError:
asset = self.asset_finder.retrieve_asset(txn.sid)
asset = self.asset_finder.retrieve_asset(txn.asset)

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

Do we still need to retrieve the asset here?

This comment has been minimized.

@ssanderson

ssanderson Apr 21, 2017

Member

(If not do we still need the multiplier caches here?)

This comment has been minimized.

@jbredeche

jbredeche Apr 23, 2017

Member

no more caches

@jbredeche jbredeche force-pushed the futures-commissions branch 2 times, most recently from 6b477bc to e82a0e6 Apr 22, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 22, 2017

Coverage Status

Coverage increased (+0.08%) to 87.612% when pulling 6b477bc on futures-commissions into 35c3cf0 on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 22, 2017

Coverage Status

Coverage increased (+0.08%) to 87.611% when pulling f4bccf0 on futures-commissions into 35c3cf0 on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 22, 2017

Coverage Status

Coverage increased (+0.08%) to 87.611% when pulling f4bccf0 on futures-commissions into 35c3cf0 on master.

@jbredeche

This comment has been minimized.

Member

jbredeche commented Apr 23, 2017

@ssanderson thanks for the review. You were completely correct about the places that were now doing redundant lookups, and I've since removed assetfinder and multiplier caching from PositionTracker and the assetfinder from PerformancePeriod.

@@ -401,6 +401,10 @@ def test_get_last_traded_dt_minute(self):
"Asset 10000 had a trade on fourth minute, so should "
"return that as the last trade on the fifth.")
def test_get_empty_splits(self):

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

👍 on having an explicit test for this case.

# send in splits for assets 133 and 2. We have no open orders for
# asset 133 so it should be ignored.
blotter.process_splits([(asset133, 0.5), (asset2, 0.3333)])
for sid in [1, 2]:

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

This could just be for asset in asset1, asset2:.

@@ -226,13 +187,12 @@ def handle_splits(self, splits):
total_leftover_cash = 0
for split in splits:
sid = split[0]
if sid in self.positions:
asset = split[0]

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

I should probably do for asset, ratio in splits here

@@ -392,15 +390,15 @@ def _calculate_execution_cash_flow(self, txn):
# Check if the multiplier is cached. If it is not, look up the asset
# and cache the multiplier.
try:
multiplier = self._execution_cash_flow_multipliers[txn.sid]
multiplier = self._execution_cash_flow_multipliers[txn.asset]

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

this _execution_cash_flow_multipliers cache can probably be removed

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

removed

cls.EQUITY2 = cls.asset_finder.retrieve_asset(2)
cls.FUTURE3 = cls.asset_finder.retrieve_asset(3)
cls.FUTURE4 = cls.asset_finder.retrieve_asset(4)
cls.FUTURE5 = cls.asset_finder.retrieve_asset(1032201401)

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Is there a reason this future is way different from the others?

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

Was just porting the existing test over. Don't know why it's such a high number.

@@ -2015,17 +2004,27 @@ def test_empty_positions(self):
self.assertNotIsInstance(val, (bool, np.bool_))
def test_position_values_and_exposures(self):
pt = perf.PositionTracker(self.env.asset_finder, None)
pt = perf.PositionTracker(None)

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

data_frequency=None seems odd here...

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

agreed. was already there, can fix later.

pos4 = perf.Position(4, amount=np.float64(-40.0),
last_sale_date=dt, last_sale_price=10)
pt.update_positions({1: pos1, 2: pos2, 3: pos3, 4: pos4})
pt.update_position(

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Is there a reason this changed from a single call to update_positions to multiple calls to update_position?

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

I removed the update_positions method, as its primary usecase was to update all those internal caches we no longer need.

I can put it back, and just have it call update_position on all the incoming positions.

@@ -665,18 +665,6 @@ class UnsupportedDatetimeFormat(ZiplineError):
"coercible to a pandas.Timestamp object.")
class PositionTrackerMissingAssetFinder(ZiplineError):

This comment has been minimized.

@ssanderson
@@ -86,9 +87,16 @@ def to_dict(self):
if self.broker_order_id is None:
del dct['broker_order_id']
dct['sid'] = self.asset

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Can we add a comment here explaining that we use sid here rather than 'asset' for backwards compat with downstream consumers of the perf packet stream?

This comment has been minimized.

@jbredeche
for position in positions:
if isinstance(position.asset, Future):
# Futures don't have an inherent position value.
values.append(0)

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

You probably want 0.0 here to avoid unnecessary coercions later.

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

good call

self._unpaid_dividends = {}
self._unpaid_stock_dividends = {}
self._positions_store = zp.Positions()
self.data_frequency = data_frequency
def _update_asset(self, sid):

This comment has been minimized.

@ssanderson
@@ -39,6 +38,9 @@ def __getitem__(self, name):
def to_dict(self):
py = copy(self.__dict__)
del py['type']
del py['asset']
py['sid'] = self.asset

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

same note here re: having a comment about this.

This comment has been minimized.

@jbredeche
self.amount = 0
self.cost_basis = 0.0 # per share
self.last_sale_price = 0.0
self.last_sale_date = None
# for backwards compatibility
self.sid = asset

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Is there a reason we're aliasing this here but making it a property in https://github.com/quantopian/zipline/pull/1757/files#diff-506712f0fea50ba9a660883d95d114d4R94 ?

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

I'd probably prefer to use a property here unless you're concerned about performance overhead. As this stands, it's possible for sid and asset to diverge if someone mutates one and not the other after construction. (Admittedly, doing that is probably a bad idea.)

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

sounds good, switched to property.

})
return txn
@expect_types(asset=Asset)
def create_txn(asset, price, amount, datetime, order_id):

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

FWIW, if you felt like being fancy, you could just write this as create_txn = expect_types(asset=Asset)(Transaction).

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

(Probably not worth it though).

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

nah, I think mine is easier to read.

txns.append(create_txn(sid, price, amount, current))
txns.append(create_txn(asset, price, amount, dt, None))

This comment has been minimized.

@ssanderson

ssanderson Apr 24, 2017

Member

Should we generate order_ids here?

This comment has been minimized.

@jbredeche

jbredeche Apr 24, 2017

Member

maybe, but it wasn't needed for my work so I didn't see the need to do it now.

@ssanderson

This comment has been minimized.

Member

ssanderson commented Apr 24, 2017

@jbredeche finished a second pass. Had a few small comments, but mostly looks good to me.

@jbredeche jbredeche force-pushed the futures-commissions branch 2 times, most recently from 18ee705 to 15d8dc9 Apr 24, 2017

@coveralls

This comment has been minimized.

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.07%) to 87.578% when pulling 15d8dc9 on futures-commissions into 123398d on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.07%) to 87.578% when pulling 15d8dc9 on futures-commissions into 123398d on master.

@coveralls

This comment has been minimized.

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.07%) to 87.578% when pulling 15d8dc9 on futures-commissions into 123398d on master.

@jbredeche jbredeche merged commit 88fc696 into master Apr 24, 2017

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 futures-commissions branch Apr 24, 2017

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