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

ENH: Use regular rounding to calculate order amounts. #1722

Merged
merged 3 commits into from Mar 28, 2017

Conversation

Projects
None yet
4 participants
@jbredeche
Member

jbredeche commented Mar 24, 2017

We previously tried to prevent accidental over-ordering by truncating
orders down unless they were within 1e-4 of the next higher integer.
Unfortunately, this makes it easy for a sell order to be one share short
of the desired position.

Using regular rounding treats both buys and sells in the same way.

@coveralls

This comment has been minimized.

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.03%) to 87.464% when pulling ba13794 on round-orders into 43d6004 on master.

@jbredeche jbredeche force-pushed the round-orders branch from ba13794 to b6b45a5 Mar 27, 2017

@jbredeche jbredeche requested a review from ssanderson Mar 27, 2017

ENH: Use regular rounding to calculate order amounts.
We previously tried to prevent accidental over-ordering by truncating
orders down unless they were within 1e-4 of the next higher integer.
Unfortunately, this makes it easy for a sell order to be one share short
of the desired position.

Using regular rounding treats both buys and sells in the same way.
@coveralls

This comment has been minimized.

coveralls commented Mar 27, 2017

Coverage Status

Coverage decreased (-0.02%) to 87.465% when pulling b6b45a5 on round-orders into 759fc5b on master.

@prsutherland prsutherland force-pushed the round-orders branch from 9771a7f to 171df4c Mar 27, 2017

@coveralls

This comment has been minimized.

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.002%) to 87.486% when pulling 171df4c on round-orders into 8c21162 on master.

@@ -1441,6 +1438,13 @@ def _calculate_order(self, asset, amount,
style)
return amount, style
@staticmethod
def _round_order(amount):

This comment has been minimized.

@ssanderson

ssanderson Mar 27, 2017

Member

Since the intent of this change is to make this method overridable by subclasses, can we make this a non-underscore method and put a docstring on it?

@ssanderson

This comment has been minimized.

Member

ssanderson commented Mar 28, 2017

LGTM (round two)

@coveralls

This comment has been minimized.

coveralls commented Mar 28, 2017

Coverage Status

Coverage increased (+0.002%) to 87.486% when pulling f9b8741 on round-orders into 8c21162 on master.

@prsutherland prsutherland merged commit 6cf81a3 into master Mar 28, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@prsutherland prsutherland deleted the round-orders branch Mar 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment