Skip to content
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

BUG: Fix rounding error in StopOrder/LimitOrder #2211

Merged
merged 19 commits into from Jul 12, 2018
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
101 changes: 100 additions & 1 deletion tests/test_execution_styles.py
Expand Up @@ -14,6 +14,7 @@
# limitations under the License.
from nose_parameterized import parameterized
from six.moves import range
import pandas as pd

from zipline.errors import BadOrderParameters
from zipline.finance.execution import (
Expand All @@ -25,10 +26,15 @@
from zipline.testing.fixtures import (
WithLogger,
ZiplineTestCase,
WithConstantFutureMinuteBarData
)

from zipline.testing.predicates import assert_equal

class ExecutionStyleTestCase(WithLogger, ZiplineTestCase):

class ExecutionStyleTestCase(WithConstantFutureMinuteBarData,
WithLogger,
ZiplineTestCase):
"""
Tests for zipline ExecutionStyle classes.
"""
Expand All @@ -46,6 +52,21 @@ class ExecutionStyleTestCase(WithLogger, ZiplineTestCase):
(0.01, 0.01, 0.01)
]

# Testing for an asset with a tick_size of 0.0001
smaller_epsilon = 0.00000001

EXPECTED_PRECISION_ROUNDING = [
(0.00, 0.00, 0.00),
(0.0005, 0.0005, 0.0005),
(0.00005, 0.00, 0.0001),
(0.000005, 0.00, 0.00),
(1.000005, 1.00, 1.00), # Lowest value to round down on sell.
(1.000005 + smaller_epsilon, 1.00, 1.0001),
(1.000095 - smaller_epsilon, 1.0, 1.0001),
(1.000095, 1.0001, 1.0001), # Highest value to round up on buy.
(0.01, 0.01, 0.01)
]

# Test that the same rounding behavior is maintained if we add between 1
# and 10 to all values, because floating point math is made of lies.
EXPECTED_PRICE_ROUNDING += [
Expand All @@ -68,6 +89,22 @@ def __str__(self):
(ArbitraryObject(),),
]

@classmethod
def make_futures_info(cls):
return pd.DataFrame.from_dict({
1: {
'multiplier': 100,
'tick_size': '0.0001',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests for tick_sizes that aren't powers of 10? I think we always want asymmetric_round_price to return a multiple of the tick_size, but for non-powers of 10, one gets:

>>> asymmetric_round_price(.0049, prefer_round_down=True, tick_size=0.005)
0.004

'symbol': 'F',
'exchange': 'TEST'
}
}, orient='index')

@classmethod
def init_class_fixtures(cls):
super(ExecutionStyleTestCase, cls).init_class_fixtures()
cls.FUTURE = cls.asset_finder.retrieve_asset(1)

@parameterized.expand(INVALID_PRICES)
def test_invalid_prices(self, price):
"""
Expand Down Expand Up @@ -155,3 +192,65 @@ def test_stop_limit_order_prices(self,
style.get_stop_price(False))
self.assertEqual(expected_limit_sell_or_stop_buy + 1,
style.get_stop_price(True))

@parameterized.expand(EXPECTED_PRECISION_ROUNDING)
def test_limit_order_precision(self,
price,
expected_limit_buy_or_stop_sell,
expected_limit_sell_or_stop_buy):
"""
Test price getters for the LimitOrder class with an asset that
has a tick_size of 0.0001.
"""
style = LimitOrder(price, asset=self.FUTURE)

assert_equal(expected_limit_buy_or_stop_sell,
style.get_limit_price(True))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the calls to style.get_limit_price and style.get_stop_price in these tests pass in constants/flags as kwargs? style.get_limit_price(is_buy=True) is easier to understand what the flag means.

assert_equal(expected_limit_sell_or_stop_buy,
style.get_limit_price(False))

assert_equal(None, style.get_stop_price(True))
assert_equal(None, style.get_stop_price(False))

@parameterized.expand(EXPECTED_PRECISION_ROUNDING)
def test_stop_order_precision(self,
price,
expected_limit_buy_or_stop_sell,
expected_limit_sell_or_stop_buy):
"""
Test price getters for StopOrder class with an asset that
has a tick_size of 0.0001. Note that the expected rounding
direction for stop prices is the reverse of that for limit prices.
"""
style = StopOrder(price, asset=self.FUTURE)

assert_equal(None, style.get_limit_price(False))
assert_equal(None, style.get_limit_price(True))

assert_equal(expected_limit_buy_or_stop_sell,
style.get_stop_price(False))
assert_equal(expected_limit_sell_or_stop_buy,
style.get_stop_price(True))

@parameterized.expand(EXPECTED_PRECISION_ROUNDING)
def test_stop_limit_order_precision(self,
price,
expected_limit_buy_or_stop_sell,
expected_limit_sell_or_stop_buy):
"""
Test price getters for StopLimitOrder class with an asset that
has a tick_size of 0.0001. Note that the expected rounding direction
for stop prices is the reverse of that for limit prices.
"""

style = StopLimitOrder(price, price + 1.00, asset=self.FUTURE)

assert_equal(expected_limit_buy_or_stop_sell,
style.get_limit_price(True))
assert_equal(expected_limit_sell_or_stop_buy,
style.get_limit_price(False))

assert_equal(expected_limit_buy_or_stop_sell + 1,
style.get_stop_price(False))
assert_equal(expected_limit_sell_or_stop_buy + 1,
style.get_stop_price(True))
19 changes: 10 additions & 9 deletions zipline/algorithm.py
Expand Up @@ -1379,10 +1379,7 @@ def _calculate_order_value_amount(self, asset, value):
# Don't place any order
return 0

if isinstance(asset, Future):
value_multiplier = asset.multiplier
else:
value_multiplier = 1
value_multiplier = asset.price_multiplier

return value / (last_price * value_multiplier)

Expand Down Expand Up @@ -1476,7 +1473,8 @@ def _calculate_order(self, asset, amount,

# Convert deprecated limit_price and stop_price parameters to use
# ExecutionStyle objects.
style = self.__convert_order_params_for_blotter(limit_price,
style = self.__convert_order_params_for_blotter(asset,
limit_price,
stop_price,
style)
return amount, style
Expand Down Expand Up @@ -1529,7 +1527,10 @@ def validate_order_params(self,
self.trading_client.current_data)

@staticmethod
def __convert_order_params_for_blotter(limit_price, stop_price, style):
def __convert_order_params_for_blotter(asset,
limit_price,
stop_price,
style):
"""
Helper method for converting deprecated limit_price and stop_price
arguments into ExecutionStyle instances.
Expand All @@ -1541,11 +1542,11 @@ def __convert_order_params_for_blotter(limit_price, stop_price, style):
assert (limit_price, stop_price) == (None, None)
return style
if limit_price and stop_price:
return StopLimitOrder(limit_price, stop_price)
return StopLimitOrder(limit_price, stop_price, asset=asset)
if limit_price:
return LimitOrder(limit_price)
return LimitOrder(limit_price, asset=asset)
if stop_price:
return StopOrder(stop_price)
return StopOrder(stop_price, asset=asset)
else:
return MarketOrder()

Expand Down
5 changes: 3 additions & 2 deletions zipline/assets/_assets.pxd
Expand Up @@ -15,6 +15,9 @@ cdef class Asset:
cdef readonly object exchange
cdef readonly object exchange_full

cdef readonly object tick_size
cdef readonly float price_multiplier

cpdef __reduce__(self)
cpdef to_dict(self)

Expand All @@ -27,8 +30,6 @@ cdef class Future(Asset):
cdef readonly object root_symbol
cdef readonly object notice_date
cdef readonly object expiration_date
cdef readonly object tick_size
cdef readonly float multiplier

cpdef __reduce__(self)
cpdef to_dict(self)
37 changes: 29 additions & 8 deletions zipline/assets/_assets.pyx
Expand Up @@ -54,6 +54,8 @@ cdef class Asset:
'auto_close_date',
'exchange',
'exchange_full',
'tick_size',
'multiplier',
})

def __init__(self,
Expand All @@ -65,7 +67,9 @@ cdef class Asset:
object end_date=None,
object first_traded=None,
object auto_close_date=None,
object exchange_full=None):
object exchange_full=None,
object tick_size="0.001",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is tick_size a string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see that it was previously defaulting to an empty string in Future below. Still not sure I understand why though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I left it as a string primarily because I noticed its default value was an empty string. Similarly, it was defined as an object rather than a float in the cython definitions.

float multiplier=1.0):

self.sid = sid
self.symbol = symbol
Expand All @@ -77,6 +81,8 @@ cdef class Asset:
self.end_date = end_date
self.first_traded = first_traded
self.auto_close_date = auto_close_date
self.tick_size = tick_size
self.price_multiplier = multiplier

def __int__(self):
return self.sid
Expand Down Expand Up @@ -144,7 +150,9 @@ cdef class Asset:
self.end_date,
self.first_traded,
self.auto_close_date,
self.exchange_full))
self.exchange_full,
self.tick_size,
self.price_multiplier))

cpdef to_dict(self):
"""
Expand All @@ -160,6 +168,8 @@ cdef class Asset:
'auto_close_date': self.auto_close_date,
'exchange': self.exchange,
'exchange_full': self.exchange_full,
'tick_size': self.tick_size,
'multiplier': self.price_multiplier,
}

@classmethod
Expand Down Expand Up @@ -254,9 +264,9 @@ cdef class Future(Asset):
'auto_close_date',
'first_traded',
'exchange',
'exchange_full',
'tick_size',
'multiplier',
'exchange_full',
})

def __init__(self,
Expand All @@ -271,7 +281,7 @@ cdef class Future(Asset):
object expiration_date=None,
object auto_close_date=None,
object first_traded=None,
object tick_size="",
object tick_size="0.001",
float multiplier=1.0,
object exchange_full=None):

Expand All @@ -285,12 +295,12 @@ cdef class Future(Asset):
first_traded=first_traded,
auto_close_date=auto_close_date,
exchange_full=exchange_full,
tick_size=tick_size,
multiplier=multiplier
)
self.root_symbol = root_symbol
self.notice_date = notice_date
self.expiration_date = expiration_date
self.tick_size = tick_size
self.multiplier = multiplier

if auto_close_date is None:
if notice_date is None:
Expand All @@ -300,6 +310,17 @@ cdef class Future(Asset):
else:
self.auto_close_date = min(notice_date, expiration_date)

property multiplier:
"""
DEPRECATION: This property should be deprecated and is only present for
backwards compatibility
"""
def __get__(self):
warnings.warn("The multiplier property will soon be "
"retired. Please use the price_multiplier property instead.",
DeprecationWarning)
return self.price_multiplier

cpdef __reduce__(self):
"""
Function used by pickle to determine how to serialize/deserialize this
Expand All @@ -319,7 +340,7 @@ cdef class Future(Asset):
self.auto_close_date,
self.first_traded,
self.tick_size,
self.multiplier,
self.price_multiplier,
self.exchange_full))

cpdef to_dict(self):
Expand All @@ -331,7 +352,7 @@ cdef class Future(Asset):
super_dict['notice_date'] = self.notice_date
super_dict['expiration_date'] = self.expiration_date
super_dict['tick_size'] = self.tick_size
super_dict['multiplier'] = self.multiplier
super_dict['multiplier'] = self.price_multiplier
return super_dict


Expand Down
2 changes: 1 addition & 1 deletion zipline/finance/_finance_ext.pyx
Expand Up @@ -207,7 +207,7 @@ cpdef calculate_position_tracker_stats(positions, PositionStats stats):
value = 0

# unchecked cast, this is safe because we do a type check above
exposure *= (<Future> position.asset).multiplier
exposure *= position.asset.price_multiplier
else:
value = exposure

Expand Down