From f62666a2242d45f1d62d13ddffd0da7bb097a747 Mon Sep 17 00:00:00 2001 From: pandashark Date: Sat, 14 Feb 2026 04:53:49 -0600 Subject: [PATCH] BUG: Fix simulation correctness bugs in slippage, cancel policy, and order params Fix execute_daily_cancel_policy to cancel all orders for all assets, not just the first order when multiple are open. Fix NoSlippage and FixedSlippage to use order.open_amount instead of order.amount. Fix VolumeShareSlippage to return (None, None) on null price. Wrap before_trading_start in try/finally to reset _in_before_trading_start flag on exception. Use 'is not None' for limit_price/stop_price checks to allow price of 0.0. Closes #313 --- docs/source/whatsnew/skeleton.txt | 16 ++++++- src/zipline/algorithm.py | 27 ++++++------ .../finance/blotter/simulation_blotter.py | 43 +------------------ src/zipline/finance/slippage.py | 6 +-- tests/test_algorithm.py | 12 +++--- 5 files changed, 39 insertions(+), 65 deletions(-) diff --git a/docs/source/whatsnew/skeleton.txt b/docs/source/whatsnew/skeleton.txt index abb397d893..b8e2e0e1c1 100644 --- a/docs/source/whatsnew/skeleton.txt +++ b/docs/source/whatsnew/skeleton.txt @@ -31,7 +31,21 @@ None Bug Fixes ~~~~~~~~~ -None +- Fix ``execute_daily_cancel_policy`` to cancel all orders for all assets at + end of day, not just the first order for assets with multiple orders. + Previously, assets with a single open order were skipped entirely. (:issue:`313`) +- Fix ``NoSlippage.process_order`` and ``FixedSlippage.process_order`` to use + ``order.open_amount`` instead of ``order.amount``, preventing overfill of + partially filled orders. (:issue:`313`) +- Fix ``VolumeShareSlippage.process_order`` to return ``(None, None)`` instead + of bare ``None`` when price is NaN, preventing ``TypeError`` during tuple + unpacking. (:issue:`313`) +- Fix ``before_trading_start`` to reset ``_in_before_trading_start`` flag via + ``try/finally``, preventing orders from being permanently blocked after an + exception in the user's callback. (:issue:`313`) +- Fix ``validate_order_params`` and ``__convert_order_params_for_blotter`` to + use ``is not None`` checks for ``limit_price`` and ``stop_price`` instead of + truthiness checks, allowing a price of ``0.0``. (:issue:`313`) Performance ~~~~~~~~~~~ diff --git a/src/zipline/algorithm.py b/src/zipline/algorithm.py index b46aade048..45f2089dd5 100644 --- a/src/zipline/algorithm.py +++ b/src/zipline/algorithm.py @@ -431,14 +431,15 @@ def before_trading_start(self, data): self._in_before_trading_start = True - with ( - handle_non_market_minutes(data) - if self.data_frequency == "minute" - else ExitStack() - ): - self._before_trading_start(self, data) - - self._in_before_trading_start = False + try: + with ( + handle_non_market_minutes(data) + if self.data_frequency == "minute" + else ExitStack() + ): + self._before_trading_start(self, data) + finally: + self._in_before_trading_start = False def handle_data(self, data): if self._handle_data: @@ -1301,12 +1302,12 @@ def validate_order_params(self, asset, amount, limit_price, stop_price, style): ) if style: - if limit_price: + if limit_price is not None: raise UnsupportedOrderParameters( msg="Passing both limit_price and style is not supported." ) - if stop_price: + if stop_price is not None: raise UnsupportedOrderParameters( msg="Passing both stop_price and style is not supported." ) @@ -1331,11 +1332,11 @@ def __convert_order_params_for_blotter(asset, limit_price, stop_price, style): if style: assert (limit_price, stop_price) == (None, None) return style - if limit_price and stop_price: + if limit_price is not None and stop_price is not None: return StopLimitOrder(limit_price, stop_price, asset=asset) - if limit_price: + if limit_price is not None: return LimitOrder(limit_price, asset=asset) - if stop_price: + if stop_price is not None: return StopOrder(stop_price, asset=asset) else: return MarketOrder() diff --git a/src/zipline/finance/blotter/simulation_blotter.py b/src/zipline/finance/blotter/simulation_blotter.py index 632de383a1..b0bbfb08fb 100644 --- a/src/zipline/finance/blotter/simulation_blotter.py +++ b/src/zipline/finance/blotter/simulation_blotter.py @@ -239,48 +239,7 @@ def execute_daily_cancel_policy(self, event): if self.cancel_policy.should_cancel(event): warn = self.cancel_policy.warn_on_cancel for asset in copy(self.open_orders): - orders = self.open_orders[asset] - if len(orders) > 1: - order = orders[0] - self.cancel(order.id, relay_status=True) - if warn: - if order.filled > 0: - warning_logger.warn( - "Your order for {order_amt} shares of " - "{order_sym} has been partially filled. " - "{order_filled} shares were successfully " - "purchased. {order_failed} shares were not " - "filled by the end of day and " - "were canceled.".format( - order_amt=order.amount, - order_sym=order.asset.symbol, - order_filled=order.filled, - order_failed=order.amount - order.filled, - ) - ) - elif order.filled < 0: - warning_logger.warn( - "Your order for {order_amt} shares of " - "{order_sym} has been partially filled. " - "{order_filled} shares were successfully " - "sold. {order_failed} shares were not " - "filled by the end of day and " - "were canceled.".format( - order_amt=order.amount, - order_sym=order.asset.symbol, - order_filled=-1 * order.filled, - order_failed=-1 * (order.amount - order.filled), - ) - ) - else: - warning_logger.warn( - "Your order for {order_amt} shares of " - "{order_sym} failed to fill by the end of day " - "and was canceled.".format( - order_amt=order.amount, - order_sym=order.asset.symbol, - ) - ) + self.cancel_all_orders_for_asset(asset, warn, relay_status=True) def execute_cancel_policy(self, event): if self.cancel_policy.should_cancel(event): diff --git a/src/zipline/finance/slippage.py b/src/zipline/finance/slippage.py index 6bc2b9c0ed..c0cd4cc89f 100644 --- a/src/zipline/finance/slippage.py +++ b/src/zipline/finance/slippage.py @@ -222,7 +222,7 @@ class NoSlippage(SlippageModel): def process_order(data, order): return ( data.current(order.asset, "close"), - order.amount, + order.open_amount, ) @@ -317,7 +317,7 @@ def process_order(self, data, order): # Remove this block after fixing data to ensure volume always has # corresponding price. if isnull(price): - return + return None, None # END simulated_impact = ( @@ -362,7 +362,7 @@ def __repr__(self): def process_order(self, data, order): price = data.current(order.asset, "close") - return (price + (self.spread / 2.0 * order.direction), order.amount) + return (price + (self.spread / 2.0 * order.direction), order.open_amount) class MarketImpactBase(SlippageModel): diff --git a/tests/test_algorithm.py b/tests/test_algorithm.py index 5b083767fc..1943556d18 100644 --- a/tests/test_algorithm.py +++ b/tests/test_algorithm.py @@ -3852,19 +3852,19 @@ def test_default_cancelation_policy(self): assert len(self._caplog.messages) == 0 def test_eod_order_cancel_daily(self): - # in daily mode, EODCancel does nothing. + # EODCancel cancels unfilled orders at end of day in daily mode. algo = self.prep_algo("set_cancel_policy(cancel_policy.EODCancel())", "daily") results = algo.run() - # order stays open throughout simulation - np.testing.assert_array_equal([1, 1, 1], list(map(len, results.orders))) + # order placed on day 0, cancelled at EOD, relay reported on day 1 + np.testing.assert_array_equal([1, 1, 0], list(map(len, results.orders))) - # one txn per day - np.testing.assert_array_equal([0, 1, 1], list(map(len, results.transactions))) + # no transactions since order is cancelled before any fill + np.testing.assert_array_equal([0, 0, 0], list(map(len, results.transactions))) with self._caplog.at_level(logging.WARNING): - assert len(self._caplog.messages) == 0 + assert len(self._caplog.messages) == 1 class TestDailyEquityAutoClose(zf.WithMakeAlgo, zf.ZiplineTestCase):