BUG: Fix rounding error in StopOrder/LimitOrder #2211

jnazaren · 2018-06-13T21:11:55Z

Solves rounding bug which caused rounding of all asset stop/limit prices to the nearest penny regardless of their tick size.

coveralls · 2018-06-13T21:41:44Z

Coverage increased (+0.06%) to 86.391% when pulling 9707d26 on rounding_bugfix into c20a588 on master.

coveralls · 2018-06-13T21:41:44Z

Coverage decreased (-0.6%) to 85.443% when pulling d433521 on rounding_bugfix into f61d3df on master.

prsutherland · 2018-06-13T21:41:18Z

zipline/finance/execution.py

@@ -158,19 +189,31 @@ def asymmetric_round_price_to_penny(price, prefer_round_down,
    If prefer_round_down: [<X-1>.0095, X.0195) -> round to X.01.
    If not prefer_round_down: (<X-1>.0005, X.0105] -> round to X.01.
    """
+    precision = get_precision(tick_size)
+    diff *= (10 ** -precision)
+    print(price, precision, diff)


While useful for debugging, we probably don't want to merge with the print.

Oh yep! Forgot to take that out, sorry

prsutherland · 2018-06-13T21:43:15Z

zipline/finance/execution.py



-def asymmetric_round_price_to_penny(price, prefer_round_down,
-                                    diff=(0.0095 - .005)):
+def asymmetric_round_price(price, prefer_round_down,


Are there tests for this method? How does it handle tick sizes like .0005?

@prsutherland I actually noticed that all of the tests that used this functionality were fine with the normal precision of this function (at least 0.01), which is why I left 0.01 as a default parameter, but I can now add some tests for smaller precision

prsutherland · 2018-06-13T21:44:48Z

zipline/finance/execution.py

-        return 0.0
+    rounded = round(price - (diff if prefer_round_down else -diff), precision)
+    # if zp_math.tolerant_equals(rounded, 0.0):
+    #     return 0.0


Is this dead code?

@prsutherland It could be, but I'm not sure if we may need it - it seems unnecessary now that precision could be as small as desired

I believe it is needed to avoid near zero floating point errors. For example, I believe without it, the code will return

>>> asymmetric_round_price(.004, True, tick_size=0.005) -0.0

Ok, I'll leave it in that case

prsutherland · 2018-06-14T16:45:14Z

zipline/finance/execution.py


    def get_limit_price(self, is_buy):
-        return asymmetric_round_price_to_penny(self.limit_price, is_buy)
+        if hasattr(self.asset, 'tick_size'):


It looks like roughly the same block is repeated ~4 times. Can the pattern be abstracted?

Would make sense to put tick_size on all asset types, with a default value? Any performance or domain modeling concerns?

prsutherland · 2018-06-26T17:57:10Z

zipline/assets/_assets.pyx

@@ -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",


Why is tick_size a string?

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

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.

prsutherland · 2018-06-26T18:05:32Z

zipline/finance/execution.py


+def _get_stop_price(asset, price, is_buy):
+    if hasattr(asset, 'tick_size'):


I think this if statement can be removed now that all assets have a tick_size.

prsutherland · 2018-06-26T18:08:00Z

zipline/protocol.py

@@ -160,8 +160,8 @@ class Order(Event):
    )


-def asset_multiplier(asset):
-    return asset.multiplier if isinstance(asset, Future) else 1
+def asset_price_multiplier(asset):


We can probably replace invocations of this method with asset.price_multiplier.

prsutherland · 2018-06-26T18:10:29Z

zipline/finance/execution.py

+        return asymmetric_round_price(
+            price,
+            not is_buy,
+            tick_size=float(asset.tick_size)


For this PR, we just cast tick_size to a float. Are there other uses that we want it to be a string?

prsutherland · 2018-06-26T18:15:04Z

tests/test_execution_styles.py

+        style = LimitOrder(price, asset=self.FUTURE)
+
+        assert_equal(expected_limit_buy_or_stop_sell,
+                     style.get_limit_price(True))


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.

prsutherland · 2018-06-26T18:25:24Z

tests/test_execution_styles.py

+        return pd.DataFrame.from_dict({
+            1: {
+                'multiplier': 100,
+                'tick_size': '0.0001',


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

prsutherland

Overall, looks good, just two small questions before merging.

prsutherland · 2018-07-05T15:36:57Z

zipline/assets/_assets.pyx

@@ -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,


Thank's for cleaning up the stringly-typed tick_size field! Can this be float tick_size now?

Sure, I can change that

prsutherland · 2018-07-05T15:39:23Z

zipline/assets/_assets.pyx

@@ -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,


For Equities, do we want a different default size? The get_stop_price and get_limit_price default to 0.01 if there is no asset.

Sure, I'll change it to 0.01

I see you updated Equity to the 0.01 tick size. Why the default value of 0.001 for futures?

A while back I had asked @ehebert for a default tick size and that's the value he gave, but I believe the default was 0.01 previously

prsutherland

This LGTM once we figure out what the defaults should be for futures and equities.

Includes a reorganization of the `Asset` class, as well as the revision in the logic of rounding in the `StopOrder`, `LimitOrder`, and `StopLimitOrder` classes. Some fields such as `price_multiplier` and `tick_size` have been moved up to the `Asset` class, and are now shared by futures and equities. Finally, all rounding done in the order classes will be based on the `tick_size` of assets.

BUG: Fix rounding error in StopOrder/LimitOrder

9707d26

jnazaren added the Bug label Jun 13, 2018

jnazaren assigned prsutherland and wulucy Jun 13, 2018

jnazaren requested a review from prsutherland June 13, 2018 21:11

prsutherland reviewed Jun 14, 2018

View reviewed changes

Jacob Nazarenko added 10 commits June 14, 2018 18:52

Remove print statement and revert dead code

44236fe

ENH: Make tick_size and price_multiplier attributes of Asset

c46a563

MAINT/STY: Fix flake8 and refactor multiplier to price_multiplier

523aee3

MAINT: Fix flake8

aff3270

MAINT: Abstract redundant code in execution

aaa6ec2

REV: Revert db schema-related files

362e83f

REV: Reverted majority of files back to 'multiplier'

99f1525

DEP: Deprecate multiplier attribute in favor of price_multiplier

90190b6

TST: Add tests for higher precision asset

4d0f510

STY: Fix flake8 issue

091606e

prsutherland reviewed Jun 26, 2018

View reviewed changes

Jacob Nazarenko added 4 commits June 28, 2018 13:36

TST: Expand and restructure execution style tests

712c3e8

TST: Fix broken tests

c1cba61

STY: Fix flake8

a4d3c00

Reorganize logic in execution

cfb67ea

prsutherland reviewed Jul 5, 2018

View reviewed changes

Jacob Nazarenko added 2 commits July 5, 2018 15:06

Merge remote-tracking branch 'origin' into rounding_bugfix

9f5cf20

MAINT: Add a different tick size for equities (0.01)

d433521

prsutherland reviewed Jul 10, 2018

View reviewed changes

Jacob Nazarenko added 2 commits July 12, 2018 13:29

REV: Redo default tick sizes for assets

783e926

Merge remote-tracking branch 'origin' into rounding_bugfix

a59aed4

jnazaren merged commit 7e642c8 into master Jul 12, 2018

jnazaren deleted the rounding_bugfix branch July 12, 2018 19:38

jnazaren mentioned this pull request Jul 12, 2018

Add descriptions of several changes #2249

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix rounding error in StopOrder/LimitOrder #2211

BUG: Fix rounding error in StopOrder/LimitOrder #2211

jnazaren commented Jun 13, 2018

coveralls commented Jun 13, 2018

coveralls commented Jun 13, 2018 •

edited

Loading

prsutherland Jun 13, 2018

jnazaren Jun 14, 2018

prsutherland Jun 13, 2018

jnazaren Jun 14, 2018 •

edited

Loading

prsutherland Jun 13, 2018

jnazaren Jun 14, 2018

prsutherland Jun 14, 2018

jnazaren Jun 14, 2018

prsutherland Jun 14, 2018

richafrank Jun 14, 2018

prsutherland Jun 26, 2018

prsutherland Jun 26, 2018

jnazaren Jun 26, 2018

prsutherland Jun 26, 2018

prsutherland Jun 26, 2018

prsutherland Jun 26, 2018

prsutherland Jun 26, 2018

prsutherland Jun 26, 2018

prsutherland left a comment

prsutherland Jul 5, 2018

jnazaren Jul 5, 2018

prsutherland Jul 5, 2018

jnazaren Jul 5, 2018

prsutherland Jul 10, 2018 •

edited

Loading

jnazaren Jul 10, 2018

prsutherland left a comment


		def _get_stop_price(asset, price, is_buy):
		if hasattr(asset, 'tick_size'):

BUG: Fix rounding error in StopOrder/LimitOrder #2211

BUG: Fix rounding error in StopOrder/LimitOrder #2211

Conversation

jnazaren commented Jun 13, 2018

coveralls commented Jun 13, 2018

coveralls commented Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnazaren Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsutherland left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsutherland Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsutherland left a comment

Choose a reason for hiding this comment

coveralls commented Jun 13, 2018 •

edited

Loading

jnazaren Jun 14, 2018 •

edited

Loading

prsutherland Jul 10, 2018 •

edited

Loading