Merge pull request #1213 from quantopian/youre-charging-me-what

DEV: Re-implement commission models to return correct results in the case of multiple fills

tests/resources/rebuild_example_data

@@ -11,9 +11,11 @@ import pandas as pd
 from zipline import examples, run_algorithm
 from zipline.testing import test_resource_path, tmp_dir
 from zipline.utils.cache import dataframe_cache
+from zipline.data.bundles import register
 
 banner = """
-Please verify that the new perfomance is more correct than the old performance.
+Please verify that the new performance is more correct than the old
+performance.
 
 To do this, please inspect `new` and `old` which are mappings from the name of
 the example to the results.
@@ -37,6 +39,9 @@ def main(ctx):
     """Rebuild the perf data for test_examples
     """
     example_path = test_resource_path('example_data.tar.gz')
+
+    register('test', lambda *args: None)
+
     with tmp_dir() as d:
         with tarfile.open(example_path) as tar:
             tar.extractall(d.path)
@@ -67,6 +72,7 @@ def main(ctx):
                     environ={
                         'ZIPLINE_ROOT': d.getpath('example_data/root'),
                     },
+                    capital_base=1e7,
                     **mod._test_args()
                 )
 

tests/test_algorithm.py

@@ -19,6 +19,7 @@ from textwrap import dedent
 from unittest import TestCase, skip
 
 import logbook
+import toolz
 from logbook import TestHandler, WARNING
 from mock import MagicMock
 from nose_parameterized import parameterized
@@ -1488,14 +1489,18 @@ def handle_data(context, data):
         self.assertEqual(len(all_txns), 1)
         txn = all_txns[0]
 
-        self.assertEqual(100.0, txn["commission"])
         expected_spread = 0.05
-        expected_commish = 0.10
-        expected_price = test_algo.recorded_vars["price"] - expected_spread \
-            - expected_commish
+        expected_price = test_algo.recorded_vars["price"] - expected_spread
 
         self.assertEqual(expected_price, txn['price'])
 
+        # make sure that the $100 commission was applied to our cash
+        # the txn was for -1000 shares at 9.95, means -9.95k.  our capital_used
+        # for that day was therefore 9.95k, but after the $100 commission,
+        # it should be 9.85k.
+        self.assertEqual(9850, results.capital_used[1])
+        self.assertEqual(100, results["orders"][1][0]["commission"])
+
     @parameterized.expand(
         [
             ('no_minimum_commission', 0,),
@@ -1543,7 +1548,6 @@ def handle_data(context, data):
                 sim_params=self.sim_params,
                 env=self.env,
             )
-            set_algo_instance(test_algo)
             trades = factory.create_daily_trade_source(
                 [0], self.sim_params, self.env)
             data_portal = create_data_portal_from_trade_history(
@@ -1555,13 +1559,28 @@ def handle_data(context, data):
                 for val in sublist]
 
             self.assertEqual(len(all_txns), 67)
-            first_txn = all_txns[0]
+            # all_orders are all the incremental versions of the
+            # orders as each new fill comes in.
+            all_orders = list(toolz.concat(results['orders']))
 
             if minimum_commission == 0:
-                commish = first_txn["amount"] * 0.02
-                self.assertEqual(commish, first_txn["commission"])
+                # for each incremental version of each order, the commission
+                # should be its filled amount * 0.02
+                for order_ in all_orders:
+                    self.assertAlmostEqual(
+                        order_["filled"] * 0.02,
+                        order_["commission"]
+                    )
             else:
-                self.assertEqual(minimum_commission, first_txn["commission"])
+                # the commission should be at least the min_trade_cost
+                for order_ in all_orders:
+                    if order_["filled"] > 0:
+                        self.assertAlmostEqual(
+                            max(order_["filled"] * 0.02, minimum_commission),
+                            order_["commission"]
+                        )
+                    else:
+                        self.assertEqual(0, order_["commission"])
         finally:
             tempdir.cleanup()
 
@@ -3046,7 +3065,7 @@ class TestEquityAutoClose(WithTmpDir, ZiplineTestCase):
             self.assertDictContainsSubset(
                 {
                     'amount': order_size,
-                    'commission': 0.0,
+                    'commission': None,
                     'dt': self.test_days[1],
                     'price': initial_fill_prices[sid],
                     'sid': sid,
@@ -3143,7 +3162,7 @@ class TestEquityAutoClose(WithTmpDir, ZiplineTestCase):
         self.assertDictContainsSubset(
             {
                 'amount': 10,
-                'commission': None,
+                'commission': 0,
                 'created': first_asset_end_date,
                 'dt': first_asset_end_date,
                 'sid': assets[0],
@@ -3158,7 +3177,7 @@ class TestEquityAutoClose(WithTmpDir, ZiplineTestCase):
         self.assertDictContainsSubset(
             {
                 'amount': 10,
-                'commission': None,
+                'commission': 0,
                 'created': first_asset_end_date,
                 'dt': first_asset_auto_close_date,
                 'sid': assets[0],
@@ -3252,7 +3271,7 @@ class TestEquityAutoClose(WithTmpDir, ZiplineTestCase):
             self.assertDictContainsSubset(
                 {
                     'amount': order_size,
-                    'commission': 0.0,
+                    'commission': None,
                     'dt': backtest_minutes[1],
                     'price': initial_fill_prices[sid],
                     'sid': sid,

tests/test_commissions.py

@@ -0,0 +1,255 @@
+from datetime import timedelta
+from textwrap import dedent
+
+from zipline import TradingAlgorithm
+from zipline.finance.commission import PerTrade, PerShare, PerDollar
+from zipline.finance.order import Order
+from zipline.finance.transaction import Transaction
+from zipline.testing import ZiplineTestCase, trades_by_sid_to_dfs
+from zipline.testing.fixtures import (
+    WithAssetFinder,
+    WithSimParams,
+    WithDataPortal
+)
+from zipline.utils import factory
+
+
+class CommissionUnitTests(WithAssetFinder, ZiplineTestCase):
+    ASSET_FINDER_EQUITY_SIDS = 1, 2
+
+    def generate_order_and_txns(self):
+        asset1 = self.asset_finder.retrieve_asset(1)
+
+        # one order
+        order = Order(dt=None, sid=asset1, amount=500)
+
+        # three fills
+        txn1 = Transaction(sid=asset1, amount=230, dt=None,
+                           price=100, order_id=order.id)
+
+        txn2 = Transaction(sid=asset1, amount=170, dt=None,
+                           price=101, order_id=order.id)
+
+        txn3 = Transaction(sid=asset1, amount=100, dt=None,
+                           price=102, order_id=order.id)
+
+        return order, [txn1, txn2, txn3]
+
+    def test_per_trade(self):
+        model = PerTrade(cost=10)
+
+        order, txns = self.generate_order_and_txns()
+
+        self.assertEqual(10, model.calculate(order, txns[0]))
+
+        order.commission = 10
+
+        self.assertEqual(0, model.calculate(order, txns[1]))
+        self.assertEqual(0, model.calculate(order, txns[2]))
+
+    def test_per_share_no_minimum(self):
+        model = PerShare(cost=0.0075, min_trade_cost=None)
+
+        order, txns = self.generate_order_and_txns()
+
+        # make sure each commission is pro-rated
+        self.assertAlmostEqual(1.725, model.calculate(order, txns[0]))
+        self.assertAlmostEqual(1.275, model.calculate(order, txns[1]))
+        self.assertAlmostEqual(0.75, model.calculate(order, txns[2]))
+
+    def verify_per_share_commissions(self, model, commission_totals):
+        order, txns = self.generate_order_and_txns()
+
+        for i, commission_total in enumerate(commission_totals):
+            order.commission += model.calculate(order, txns[i])
+            self.assertAlmostEqual(commission_total, order.commission)
+            order.filled += txns[i].amount
+
+    def test_per_share_with_minimum(self):
+        # minimum is met by the first trade
+        self.verify_per_share_commissions(
+            PerShare(cost=0.0075, min_trade_cost=1),
+            [1.725, 3, 3.75]
+        )
+
+        # minimum is met by the second trade
+        self.verify_per_share_commissions(
+            PerShare(cost=0.0075, min_trade_cost=2.5),
+            [2.5, 3, 3.75]
+        )
+
+        # minimum is met by the third trade
+        self.verify_per_share_commissions(
+            PerShare(cost=0.0075, min_trade_cost=3.5),
+            [3.5, 3.5, 3.75]
+        )
+
+        # minimum is not met by any of the trades
+        self.verify_per_share_commissions(
+            PerShare(cost=0.0075, min_trade_cost=5.5),
+            [5.5, 5.5, 5.5]
+        )
+
+    def test_per_dollar(self):
+        model = PerDollar(cost=0.0015)
+
+        order, txns = self.generate_order_and_txns()
+
+        # make sure each commission is pro-rated
+        self.assertAlmostEqual(34.5, model.calculate(order, txns[0]))
+        self.assertAlmostEqual(25.755, model.calculate(order, txns[1]))
+        self.assertAlmostEqual(15.3, model.calculate(order, txns[2]))
+
+
+class CommissionAlgorithmTests(WithDataPortal, WithSimParams, ZiplineTestCase):
+    # make sure order commissions are properly incremented
+
+    sidint, = ASSET_FINDER_EQUITY_SIDS = (133,)
+
+    code = dedent(
+        """
+        from zipline.api import (
+            sid, order, set_slippage, slippage, FixedSlippage,
+            set_commission, commission
+        )
+
+        def initialize(context):
+            # for these tests, let us take out the entire bar with no price
+            # impact
+            set_slippage(slippage.VolumeShareSlippage(1.0, 0))
+
+            {0}
+            context.ordered = False
+
+
+        def handle_data(context, data):
+            if not context.ordered:
+                order(sid(133), {1})
+                context.ordered = True
+        """,
+    )
+
+    @classmethod
+    def make_daily_bar_data(cls):
+        num_days = len(cls.sim_params.trading_days)
+
+        return trades_by_sid_to_dfs(
+            {
+                cls.sidint: factory.create_trade_history(
+                    cls.sidint,
+                    [10.0] * num_days,
+                    [100.0] * num_days,
+                    timedelta(days=1),
+                    cls.sim_params,
+                    cls.env,
+                ),
+            },
+            index=cls.sim_params.trading_days,
+        )
+
+    def get_results(self, algo_code):
+        algo = TradingAlgorithm(
+            script=algo_code,
+            env=self.env,
+            sim_params=self.sim_params
+        )
+
+        return algo.run(self.data_portal)
+
+    def test_per_trade(self):
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerTrade(1))", 300)
+        )
+
+        # should be 3 fills at 100 shares apiece
+        # one order split among 3 days, each copy of the order should have a
+        # commission of one dollar
+        for orders in results.orders[1:4]:
+            self.assertEqual(1, orders[0]["commission"])
+
+        self.verify_capital_used(results, [-1001, -1000, -1000])
+
+    def test_per_share_no_minimum(self):
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerShare(0.05, None))",
+                             300)
+        )
+
+        # should be 3 fills at 100 shares apiece
+        # one order split among 3 days, each fill generates an additional
+        # 100 * 0.05 = $5 in commission
+        for i, orders in enumerate(results.orders[1:4]):
+            self.assertEqual((i + 1) * 5, orders[0]["commission"])
+
+        self.verify_capital_used(results, [-1005, -1005, -1005])
+
+    def test_per_share_with_minimum(self):
+        # minimum hit by first trade
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerShare(0.05, 3))",
+                             300)
+        )
+
+        # commissions should be 5, 10, 15
+        for i, orders in enumerate(results.orders[1:4]):
+            self.assertEqual((i + 1) * 5, orders[0]["commission"])
+
+        self.verify_capital_used(results, [-1005, -1005, -1005])
+
+        # minimum hit by second trade
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerShare(0.05, 8))",
+                             300)
+        )
+
+        # commissions should be 8, 10, 15
+        self.assertEqual(8, results.orders[1][0]["commission"])
+        self.assertEqual(10, results.orders[2][0]["commission"])
+        self.assertEqual(15, results.orders[3][0]["commission"])
+
+        self.verify_capital_used(results, [-1008, -1002, -1005])
+
+        # minimum hit by third trade
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerShare(0.05, 12))",
+                             300)
+        )
+
+        # commissions should be 12, 12, 15
+        self.assertEqual(12, results.orders[1][0]["commission"])
+        self.assertEqual(12, results.orders[2][0]["commission"])
+        self.assertEqual(15, results.orders[3][0]["commission"])
+
+        self.verify_capital_used(results, [-1012, -1000, -1003])
+
+        # minimum never hit
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerShare(0.05, 18))",
+                             300)
+        )
+
+        # commissions should be 18, 18, 18
+        self.assertEqual(18, results.orders[1][0]["commission"])
+        self.assertEqual(18, results.orders[2][0]["commission"])
+        self.assertEqual(18, results.orders[3][0]["commission"])
+
+        self.verify_capital_used(results, [-1018, -1000, -1000])
+
+    def test_per_dollar(self):
+        results = self.get_results(
+            self.code.format("set_commission(commission.PerDollar(0.01))", 300)
+        )
+
+        # should be 3 fills at 100 shares apiece, each fill is worth $1k, so
+        # incremental commission of $1000 * 0.01 = $10
+
+        # commissions should be $10, $20, $30
+        for i, orders in enumerate(results.orders[1:4]):
+            self.assertEqual((i + 1) * 10, orders[0]["commission"])
+
+        self.verify_capital_used(results, [-1010, -1010, -1010])
+
+    def verify_capital_used(self, results, values):
+        self.assertEqual(values[0], results.capital_used[1])
+        self.assertEqual(values[1], results.capital_used[2])
+        self.assertEqual(values[2], results.capital_used[3])

tests/test_perf_tracking.py

@@ -41,7 +41,6 @@ from zipline.finance.transaction import create_transaction
 import zipline.utils.math_utils as zp_math
 
 from zipline.finance.blotter import Order
-from zipline.finance.commission import PerShare, PerTrade, PerDollar
 from zipline.finance.performance.position import Position
 from zipline.utils.factory import create_simulation_parameters
 from zipline.utils.serialization_utils import (
@@ -392,183 +391,6 @@ class TestSplitPerformance(WithSimParams, WithTmpDir, ZiplineTestCase):
                                  (i, perf_kind, perf_result['returns']))
 
 
-class TestCommissionEvents(WithSimParams, WithTmpDir, ZiplineTestCase):
-    START_DATE = pd.Timestamp('2006-01-03', tz='utc')
-    END_DATE = pd.Timestamp('2006-01-09', tz='utc')
-    ASSET_FINDER_EQUITY_SIDS = 0, 1, 133
-    SIM_PARAMS_CAPITAL_BASE = 10e3
-
-    @classmethod
-    def init_class_fixtures(cls):
-        super(TestCommissionEvents, cls).init_class_fixtures()
-        cls.asset1 = cls.env.asset_finder.retrieve_asset(1)
-
-    def test_commission_event(self):
-        trade_events = factory.create_trade_history(
-            self.asset1,
-            [10, 10, 10, 10, 10],
-            [100, 100, 100, 100, 100],
-            oneday,
-            self.sim_params,
-            env=self.env
-        )
-
-        # Test commission models and validate result
-        # Expected commission amounts:
-        # PerShare commission:  1.00, 1.00, 1.50 = $3.50
-        # PerTrade commission:  5.00, 5.00, 5.00 = $15.00
-        # PerDollar commission: 1.50, 3.00, 4.50 = $9.00
-        # Total commission = $3.50 + $15.00 + $9.00 = $27.50
-
-        data_portal = create_data_portal_from_trade_history(
-            self.env,
-            self.tmpdir,
-            self.sim_params,
-            {1: trade_events},
-        )
-
-        # Create 3 transactions:  50, 100, 150 shares traded @ $20
-        first_trade = trade_events[0]
-        transactions = [create_txn(first_trade.sid, first_trade.dt, 20, i)
-                        for i in [50, 100, 150]]
-
-        # Create commission models and validate that produce expected
-        # commissions.
-        models = [PerShare(cost=0.01, min_trade_cost=1.00),
-                  PerTrade(cost=5.00),
-                  PerDollar(cost=0.0015)]
-        expected_results = [3.50, 15.0, 9.0]
-
-        for model, expected in zip(models, expected_results):
-            total_commission = 0
-            for trade in transactions:
-                total_commission += model.calculate(trade)[1]
-            self.assertEqual(total_commission, expected)
-
-        # Verify that commission events are handled correctly by
-        # PerformanceTracker.
-        commissions = {}
-        cash_adj_dt = trade_events[0].dt
-        cash_adjustment = factory.create_commission(1, 300.0, cash_adj_dt)
-        commissions[cash_adj_dt] = [cash_adjustment]
-
-        # Insert a purchase order.
-        txns = [create_txn(first_trade.sid, first_trade.dt, 20, 1)]
-        results = calculate_results(self.sim_params,
-                                    self.env,
-                                    data_portal,
-                                    txns=txns,
-                                    commissions=commissions)
-
-        # Validate that we lost 320 dollars from our cash pool.
-        self.assertEqual(results[-1]['cumulative_perf']['ending_cash'],
-                         9680, "Should have lost 320 from cash pool.")
-        # Validate that the cost basis of our position changed.
-        self.assertEqual(results[-1]['daily_perf']['positions']
-                         [0]['cost_basis'], 320.0)
-        # Validate that the account attributes were updated.
-        account = results[1]['account']
-        self.assertEqual(float('inf'), account['day_trades_remaining'])
-        np.testing.assert_allclose(0.001, account['leverage'], rtol=1e-3,
-                                   atol=1e-4)
-        np.testing.assert_allclose(9680, account['regt_equity'], rtol=1e-3)
-        self.assertEqual(float('inf'), account['regt_margin'])
-        np.testing.assert_allclose(9680, account['available_funds'],
-                                   rtol=1e-3)
-        self.assertEqual(0, account['maintenance_margin_requirement'])
-        np.testing.assert_allclose(9690,
-                                   account['equity_with_loan'], rtol=1e-3)
-        self.assertEqual(float('inf'), account['buying_power'])
-        self.assertEqual(0, account['initial_margin_requirement'])
-        np.testing.assert_allclose(9680, account['excess_liquidity'],
-                                   rtol=1e-3)
-        np.testing.assert_allclose(9680, account['settled_cash'],
-                                   rtol=1e-3)
-        np.testing.assert_allclose(9690, account['net_liquidation'],
-                                   rtol=1e-3)
-        np.testing.assert_allclose(0.999, account['cushion'], rtol=1e-3)
-        np.testing.assert_allclose(10, account['total_positions_value'],
-                                   rtol=1e-3)
-        self.assertEqual(0, account['accrued_interest'])
-
-    def test_commission_zero_position(self):
-        """
-        Ensure no div-by-zero errors.
-        """
-        events = factory.create_trade_history(
-            self.asset1,
-            [10, 10, 10, 10, 10],
-            [100, 100, 100, 100, 100],
-            oneday,
-            self.sim_params,
-            env=self.env
-        )
-
-        data_portal = create_data_portal_from_trade_history(
-            self.env,
-            self.tmpdir,
-            self.sim_params,
-            {1: events},
-        )
-
-        # Buy and sell the same sid so that we have a zero position by the
-        # time of events[3].
-        txns = [
-            create_txn(self.asset1, events[0].dt, 20, 1),
-            create_txn(self.asset1, events[0].dt, 20, -1)
-        ]
-
-        # Add a cash adjustment at the time of event[3].
-        cash_adj_dt = events[3].dt
-        commissions = {}
-        cash_adjustment = factory.create_commission(1, 300.0, cash_adj_dt)
-        commissions[cash_adj_dt] = [cash_adjustment]
-
-        results = calculate_results(self.sim_params,
-                                    self.env,
-                                    data_portal,
-                                    txns=txns,
-                                    commissions=commissions)
-        # Validate that we lost 300 dollars from our cash pool.
-        self.assertEqual(results[-1]['cumulative_perf']['ending_cash'],
-                         9700)
-
-    def test_commission_no_position(self):
-        """
-        Ensure no position-not-found or sid-not-found errors.
-        """
-        events = factory.create_trade_history(
-            self.asset1,
-            [10, 10, 10, 10, 10],
-            [100, 100, 100, 100, 100],
-            oneday,
-            self.sim_params,
-            env=self.env
-        )
-
-        data_portal = create_data_portal_from_trade_history(
-            self.env,
-            self.tmpdir,
-            self.sim_params,
-            {1: events},
-        )
-
-        # Add a cash adjustment at the time of event[3].
-        cash_adj_dt = events[3].dt
-        commissions = {}
-        cash_adjustment = factory.create_commission(self.asset1,
-                                                    300.0, cash_adj_dt)
-        commissions[cash_adj_dt] = [cash_adjustment]
-
-        results = calculate_results(self.sim_params,
-                                    self.env,
-                                    data_portal,
-                                    commissions=commissions)
-        # Validate that we lost 300 dollars from our cash pool.
-        self.assertEqual(results[-1]['cumulative_perf']['ending_cash'],
-                         9700)
-
-
 class TestDividendPerformance(WithSimParams,
                               WithInstanceTmpDir,
                               ZiplineTestCase):

zipline/algorithm.py

@@ -56,7 +56,7 @@ from zipline.errors import (
     OrderInBeforeTradingStart)
 from zipline.finance.trading import TradingEnvironment
 from zipline.finance.blotter import Blotter
-from zipline.finance.commission import PerShare, PerTrade, PerDollar
+from zipline.finance.commission import PerShare, CommissionModel
 from zipline.finance.controls import (
     LongOnly,
     MaxOrderCount,
@@ -1483,11 +1483,11 @@ class TradingAlgorithm(object):
 
     @api_method
     def set_commission(self, commission):
-        """Sets the commision model for the simulation.
+        """Sets the commission model for the simulation.
 
         Parameters
         ----------
-        commission : PerShare, PerTrade, or PerDollar
+        commission : CommissionModel
             The commission model to use.
 
         See Also
@@ -1496,11 +1496,12 @@ class TradingAlgorithm(object):
         :class:`zipline.finance.commission.PerTrade`
         :class:`zipline.finance.commission.PerDollar`
         """
-        if not isinstance(commission, (PerShare, PerTrade, PerDollar)):
+        if not isinstance(commission, CommissionModel):
             raise UnsupportedCommissionModel()
 
         if self.initialized:
             raise SetCommissionPostInit()
+
         self.blotter.commission = commission
 
     @api_method

zipline/finance/blotter.py

@@ -12,17 +12,13 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
-import math
-
 from logbook import Logger
 from collections import defaultdict
 from copy import copy
 
-import pandas as pd
 from six import iteritems
 
 from zipline.finance.order import Order
-
 from zipline.finance.slippage import VolumeShareSlippage
 from zipline.finance.commission import PerShare
 from zipline.finance.cancel_policy import NeverCancel
@@ -293,9 +289,7 @@ class Blotter(object):
         commissions_list: List
             commissions_list: list of commissions resulting from filling the
             open orders.  A commission is an object with "sid" and "cost"
-            parameters.  If there are no commission events (because, for
-            example, Zipline models the commission cost into the fill price
-            of the transaction), then this is None.
+            parameters.
 
         closed_orders: List
             closed_orders: list of all the orders that have filled.
@@ -303,6 +297,7 @@ class Blotter(object):
 
         closed_orders = []
         transactions = []
+        commissions = []
 
         if self.open_orders:
             assets = self.asset_finder.retrieve_all(self.open_orders)
@@ -313,18 +308,19 @@ class Blotter(object):
 
                 for order, txn in \
                         self.slippage_func(bar_data, asset, asset_orders):
-                    direction = math.copysign(1, txn.amount)
-                    per_share, total_commission = \
-                        self.commission.calculate(txn)
-                    txn.price += per_share * direction
-                    txn.commission = total_commission
+                    additional_commission = \
+                        self.commission.calculate(order, txn)
+
+                    if additional_commission > 0:
+                        commissions.append({
+                            "sid": order.sid,
+                            "order": order,
+                            "cost": additional_commission
+                        })
+
                     order.filled += txn.amount
+                    order.commission += additional_commission
 
-                    if txn.commission is not None:
-                        order.commission = (order.commission or 0.0) + \
-                            txn.commission
-
-                    txn.dt = pd.Timestamp(txn.dt, tz='UTC')
                     order.dt = txn.dt
 
                     transactions.append(txn)
@@ -332,7 +328,7 @@ class Blotter(object):
                     if not order.open:
                         closed_orders.append(order)
 
-        return transactions, None, closed_orders
+        return transactions, commissions, closed_orders
 
     def prune_orders(self, closed_orders):
         """

zipline/finance/commission.py

@@ -1,5 +1,5 @@
 #
-# Copyright 2014 Quantopian, Inc.
+# Copyright 2016 Quantopian, Inc.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -12,113 +12,160 @@
 # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 # See the License for the specific language governing permissions and
 # limitations under the License.
+import abc
+
+from abc import abstractmethod
+from six import with_metaclass
 
 DEFAULT_PER_SHARE_COST = 0.0075         # 0.75 cents per share
 DEFAULT_MINIMUM_COST_PER_TRADE = 1.0    # $1 per trade
 
 
-class PerShare(object):
-    """Calculates a commission for a transaction based on a per
-    share cost with an optional minimum cost per trade.
+class CommissionModel(with_metaclass(abc.ABCMeta)):
+    """
+    Abstract commission model interface.
+    """
+
+    @abstractmethod
+    def calculate(self, order, transaction):
+        """
+        Parameters
+        ----------
+        order: the order whose transaction we are processing.  Its `commission`
+            field is up to date with how much commission has been attributed
+            to this order so far.
+
+        transaction: the transaction we are processing.
+
+        Returns
+        -------
+        float: The additional commission, in dollars, that we should attribute
+            to this order.
+        """
+        pass
+
+
+class PerShare(CommissionModel):
+    """
+    Calculates a commission for a transaction based on a per share cost with
+    an optional minimum cost per trade.
 
     Parameters
     ----------
     cost : float, optional
         The amount of commissions paid per share traded.
     min_trade_cost : optional
-        The minimum amount of commisions paid per trade.
+        The minimum amount of commissions paid per trade.
     """
 
     def __init__(self,
                  cost=DEFAULT_PER_SHARE_COST,
                  min_trade_cost=DEFAULT_MINIMUM_COST_PER_TRADE):
-        """
-        Cost parameter is the cost of a trade per-share. $0.03
-        means three cents per share, which is a very conservative
-        (quite high) for per share costs.
-        min_trade_cost parameter is the minimum trade cost
-        regardless of the number of shares traded (e.g. $1.00).
-        """
-        self.cost = float(cost)
-        self.min_trade_cost = None if min_trade_cost is None\
-            else float(min_trade_cost)
+        self.cost_per_share = float(cost)
+        self.min_trade_cost = min_trade_cost
 
     def __repr__(self):
-        return "{class_name}(cost={cost}, min trade cost={min_trade_cost})"\
+        return "{class_name}(cost_per_share={cost}, " \
+               "min trade cost={min_trade_cost})" \
             .format(class_name=self.__class__.__name__,
-                    cost=self.cost,
+                    cost_per_share=self.cost_per_share,
                     min_trade_cost=self.min_trade_cost)
 
-    def calculate(self, transaction):
+    def calculate(self, order, transaction):
         """
-        returns a tuple of:
-        (per share commission, total transaction commission)
+        If there is a minimum commission:
+            If the order hasn't had a commission paid yet, pay the minimum
+            commission.
+
+            If the order has paid a commission, start paying additional
+            commission once the minimum commission has been reached.
+
+        If there is no minimum commission:
+            Pay commission based on number of shares in the transaction.
         """
-        commission = abs(transaction.amount * self.cost)
+        additional_commission = abs(transaction.amount * self.cost_per_share)
+
         if self.min_trade_cost is None:
-            return self.cost, commission
+            # no min trade cost, so just return the cost for this transaction
+            return additional_commission
+
+        if order.commission == 0:
+            # no commission paid yet, pay at least the minimum
+            return max(self.min_trade_cost, additional_commission)
         else:
-            commission = max(commission, self.min_trade_cost)
-            return abs(commission / transaction.amount), commission
+            # we've already paid some commission, so figure out how much we
+            # would be paying if we only counted per share.
+            per_share_total = \
+                (order.filled * self.cost_per_share) + additional_commission
+
+            if per_share_total < self.min_trade_cost:
+                # if we haven't hit the minimum threshold yet, don't pay
+                # additional commission
+                return 0
+            else:
+                # we've exceeded the threshold, so pay more commission.
+                return per_share_total - order.commission
 
 
-class PerTrade(object):
-    """Calculates a commission for a transaction based on a per
-    trade cost.
+class PerTrade(CommissionModel):
+    """
+    Calculates a commission for a transaction based on a per trade cost.
 
     Parameters
     ----------
     cost : float, optional
-        The flat amount of commisions paid per trade.
+        The flat amount of commissions paid per trade.
     """
 
     def __init__(self, cost=DEFAULT_MINIMUM_COST_PER_TRADE):
         """
-        Cost parameter is the cost of a trade, regardless of
-        share count. $5.00 per trade is fairly typical of
-        discount brokers.
+        Cost parameter is the cost of a trade, regardless of share count.
+        $5.00 per trade is fairly typical of discount brokers.
         """
         # Cost needs to be floating point so that calculation using division
         # logic does not floor to an integer.
         self.cost = float(cost)
 
-    def calculate(self, transaction):
+    def calculate(self, order, transaction):
         """
-        returns a tuple of:
-        (per share commission, total transaction commission)
+        If the order hasn't had a commission paid yet, pay the fixed
+        commission.
         """
-        if transaction.amount == 0:
-            return 0.0, 0.0
-
-        return abs(self.cost / transaction.amount), self.cost
+        if order.commission == 0:
+            # if the order hasn't had a commission attributed to it yet,
+            # that's what we need to pay.
+            return self.cost
+        else:
+            # order has already had commission attributed, so no more
+            # commission.
+            return 0.0
 
 
-class PerDollar(object):
-    """Calculates a commission for a transaction based on a per
-    dollar cost.
+class PerDollar(CommissionModel):
+    """
+    Calculates a commission for a transaction based on a per trade cost.
 
     Parameters
     ----------
-    cost : float, optional
-        The amount of commissions paid per dollar traded.
+    cost : float
+        The flat amount of commissions paid per trade.
     """
 
     def __init__(self, cost=0.0015):
         """
         Cost parameter is the cost of a trade per-dollar. 0.0015
-        on $1 million means $1,500 commission (=1,000,000 x 0.0015)
+        on $1 million means $1,500 commission (=1M * 0.0015)
         """
-        self.cost = float(cost)
+        self.cost_per_dollar = float(cost)
 
     def __repr__(self):
-        return "{class_name}(cost={cost})".format(
+        return "{class_name}(cost_per_dollar={cost})".format(
             class_name=self.__class__.__name__,
-            cost=self.cost)
+            cost=self.cost_per_dollar)
 
-    def calculate(self, transaction):
+    def calculate(self, order, transaction):
         """
-        returns a tuple of:
-        (per share commission, total transaction commission)
+        Pay commission based on dollar value of shares.
         """
-        cost_per_share = transaction.price * self.cost
-        return cost_per_share, abs(transaction.amount) * cost_per_share
+        cost_per_share = transaction.price * self.cost_per_dollar
+        return abs(transaction.amount) * cost_per_share

zipline/finance/order.py

@@ -46,7 +46,7 @@ class Order(object):
                  "limit_reached", "direction", "type", "broker_order_id"]
 
     def __init__(self, dt, sid, amount, stop=None, limit=None, filled=0,
-                 commission=None, id=None):
+                 commission=0, id=None):
         """
         @dt - datetime.datetime that the order was placed
         @sid - asset for the order.  called sid for historical reasons.