From a9c0ce1ddecdf9d3cec478ffe90cbcc7c14e7344 Mon Sep 17 00:00:00 2001
From: Jean Bredeche <jean@quantopian.com>
Date: Tue, 17 Jan 2017 21:00:22 -0500
Subject: [PATCH] ENH: Small refactoring of fill price check.

---
 tests/finance/test_slippage.py | 27 +++++++++++++++++--
 zipline/finance/slippage.py    | 49 +++++++++++++++++++++++++---------
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/tests/finance/test_slippage.py b/tests/finance/test_slippage.py
index ba1ac919..d1992f09 100644
--- a/tests/finance/test_slippage.py
+++ b/tests/finance/test_slippage.py
@@ -1,5 +1,5 @@
 #
-# Copyright 2013 Quantopian, Inc.
+# Copyright 2017 Quantopian, Inc.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -17,6 +17,7 @@
 Unit tests for finance.slippage
 '''
 import datetime
+from collections import namedtuple
 
 import pytz
 
@@ -25,7 +26,8 @@ from nose_parameterized import parameterized
 import pandas as pd
 from pandas.tslib import normalize_date
 
-from zipline.finance.slippage import VolumeShareSlippage
+from zipline.finance.slippage import VolumeShareSlippage, \
+    fill_price_worse_than_limit_price
 
 from zipline.protocol import DATASOURCE_TYPE, BarData
 from zipline.finance.blotter import Order
@@ -42,6 +44,9 @@ from zipline.testing.fixtures import (
 from zipline.utils.classproperty import classproperty
 
 
+TestOrder = namedtuple('TestOrder', 'limit direction')
+
+
 class SlippageTestCase(WithCreateBarData,
                        WithSimParams,
                        WithDataPortal,
@@ -83,6 +88,24 @@ class SlippageTestCase(WithCreateBarData,
         super(SlippageTestCase, cls).init_class_fixtures()
         cls.ASSET133 = cls.env.asset_finder.retrieve_asset(133)
 
+    def test_fill_price_worse_than_limit_price(self):
+        non_limit_order = TestOrder(limit=None, direction=1)
+        limit_buy = TestOrder(limit=1.5, direction=1)
+        limit_sell = TestOrder(limit=1.5, direction=-1)
+
+        for price in [1, 1.5, 2]:
+            self.assertFalse(
+                fill_price_worse_than_limit_price(price, non_limit_order)
+            )
+
+        self.assertFalse(fill_price_worse_than_limit_price(1, limit_buy))
+        self.assertFalse(fill_price_worse_than_limit_price(1.5, limit_buy))
+        self.assertTrue(fill_price_worse_than_limit_price(2, limit_buy))
+
+        self.assertTrue(fill_price_worse_than_limit_price(1, limit_sell))
+        self.assertFalse(fill_price_worse_than_limit_price(1.5, limit_sell))
+        self.assertFalse(fill_price_worse_than_limit_price(2, limit_sell))
+
     def test_orders_limit(self):
         slippage_model = VolumeShareSlippage()
         slippage_model.data_portal = self.data_portal
diff --git a/zipline/finance/slippage.py b/zipline/finance/slippage.py
index db5781dc..20beccfd 100644
--- a/zipline/finance/slippage.py
+++ b/zipline/finance/slippage.py
@@ -1,5 +1,5 @@
 #
-# Copyright 2015 Quantopian, Inc.
+# Copyright 2017 Quantopian, Inc.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -35,6 +35,39 @@ class LiquidityExceeded(Exception):
 DEFAULT_VOLUME_SLIPPAGE_BAR_LIMIT = 0.025
 
 
+def fill_price_worse_than_limit_price(fill_price, order):
+    """
+    Checks whether the fill price is worse than the order's limit price.
+
+    Parameters
+    ----------
+    fill_price: float
+        The price to check.
+
+    order: zipline.finance.order.Order
+        The order whose limit price to check.
+
+    Returns
+    -------
+    bool: Whether the fill price is above the limit price (for a buy) or below
+    the limit price (for a sell).
+    """
+    if order.limit:
+        # this is tricky! if an order with a limit price has reached
+        # the limit price, we will try to fill the order. do not fill
+        # these shares if the impacted price is worse than the limit
+        # price. return early to avoid creating the transaction.
+
+        # buy order is worse if the impacted price is greater than
+        # the limit price. sell order is worse if the impacted price
+        # is less than the limit price
+        if (order.direction > 0 and fill_price > order.limit) or \
+                (order.direction < 0 and fill_price < order.limit):
+            return True
+
+    return False
+
+
 class SlippageModel(with_metaclass(abc.ABCMeta)):
     """Abstract interface for defining a slippage model.
     """
@@ -182,18 +215,8 @@ class VolumeShareSlippage(SlippageModel):
             * price
         impacted_price = price + simulated_impact
 
-        if order.limit:
-            # this is tricky! if an order with a limit price has reached
-            # the limit price, we will try to fill the order. do not fill
-            # these shares if the impacted price is worse than the limit
-            # price. return early to avoid creating the transaction.
-
-            # buy order is worse if the impacted price is greater than
-            # the limit price. sell order is worse if the impacted price
-            # is less than the limit price
-            if (order.direction > 0 and impacted_price > order.limit) or \
-                    (order.direction < 0 and impacted_price < order.limit):
-                return None, None
+        if fill_price_worse_than_limit_price(impacted_price, order):
+            return None, None
 
         return (
             impacted_price,