From ecd9bff0d601b4b62ddc889a73cf555a13458545 Mon Sep 17 00:00:00 2001 From: Scott Sanderson Date: Mon, 26 May 2014 17:44:13 -0400 Subject: [PATCH] PERF/BUG: Make the portfolio property call `updated_portfolio`. Make the portfolio property on TradingAlgorithm call `updated_portfolio` internally. This prevents needless recomputation of the portfolio between calls to `handle_data`, and also prevents issues where the portfolio object could be unexpectedly modified in place in the body of a `handle_data` call. Noteworthy finding in the course of investigating this bug: If you modify a Python dictionary while iterating over it, the language will only throw an exception if the size of the dictionary changes between loop iterations; this means that you can do: ``` x = {1:1, 2:2, 3:3} for k in x: old_val = x[k] del x[k] x[f(k)] = old_val print k ``` and you'll only get an error if f(k) is already a key in the dictionary. This can lead to bizarre/nondeterministic behavior in the key iterator. --- zipline/algorithm.py | 6 +----- zipline/finance/performance/period.py | 24 ++++++++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/zipline/algorithm.py b/zipline/algorithm.py index 1e18bba2..4634c862 100644 --- a/zipline/algorithm.py +++ b/zipline/algorithm.py @@ -601,13 +601,9 @@ class TradingAlgorithm(object): @property def portfolio(self): - # internally this will cause a refresh of the - # period performance calculations. - return self.perf_tracker.get_portfolio() + return self.updated_portfolio() def updated_portfolio(self): - # internally this will cause a refresh of the - # period performance calculations. if self.portfolio_needs_update: self._portfolio = self.perf_tracker.get_portfolio() self.portfolio_needs_update = False diff --git a/zipline/finance/performance/period.py b/zipline/finance/performance/period.py index a9b38dec..7b51effc 100644 --- a/zipline/finance/performance/period.py +++ b/zipline/finance/performance/period.py @@ -367,20 +367,24 @@ class PerformancePeriod(object): positions = self._positions_store for sid, pos in iteritems(self.positions): - if pos.amount != 0 and sid not in positions: - positions[sid] = zp.Position(sid) + + if pos.amount == 0: + # Clear out the position if it has become empty since the last + # time get_positions was called. Catching the KeyError is + # faster than checking `if sid in positions`, and this can be + # potentially called in a tight inner loop. + try: + del positions[sid] + except KeyError: + pass + continue + + # Note that this will create a position if we don't currently have + # an entry position = positions[sid] position.amount = pos.amount position.cost_basis = pos.cost_basis position.last_sale_price = pos.last_sale_price - - # Remove positions with no amounts from portfolio container - # that is passed to the algorithm. - # These positions are still stored internally for use with - # dividends etc. - if pos.amount == 0 and sid in positions: - del positions[sid] - return positions def get_positions_list(self):