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

allow order_percent to work with various market values #477

Closed
wants to merge 15 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@jlowin
Contributor

jlowin commented Jan 28, 2015

Currently, order_percent() and order_target_percent() both operate as a percentage of self.portfolio.portfolio_value. This PR lets them operate as percentages of other important MVs.

(also adds context.get_market_value(), which enables this functionality)

For example:

# this is how it works today (and this still works)
# put 50% of my portfolio in AAPL
order_percent('AAPL', 0.5)
# note that if this were a fully invested portfolio, it would become 150% levered.

# take half of my available cash and buy AAPL
order_percent('AAPL', 0.5, percent_of='cash')

# rebalance my short position, as a percentage of my current short book
order_target_percent('MSFT', 0.1, percent_of='shorts')

# rebalance within a custom group of stocks
tech_stocks = ('AAPL', 'MSFT', 'GOOGL')
tech_filter = lambda p: p.sid in tech_stocks
for stock in tech_stocks:
    order_target_percent(stock, 1/3, percent_of_fn=tech_filter)

Jeremiah Lowin added some commits Jan 28, 2015

@coveralls

This comment has been minimized.

coveralls commented Jan 29, 2015

Coverage Status

Coverage decreased (-0.17%) to 86.75% when pulling 91abb42 on jlowin:mod-order_percent into 65c038d on quantopian:master.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jan 29, 2015

This looks great, thanks @jlowin! I'll take a more detailed look later but one thing I wonder about is unittests. Do you plan to still add those?

@jlowin

This comment has been minimized.

Contributor

jlowin commented Jan 30, 2015

@twiecki Yes, hopefully soon.

@jlowin

This comment has been minimized.

Contributor

jlowin commented Feb 7, 2015

@twiecki added tests.

Along the way, I discovered a problem with order_target() (and its unit tests) that I've fixed here.

The main problem was that order_target could arrive at an unexpected number of target_shares.

For example, consider:

  1. I have 0 shares
  2. I call order_target(X, 49.9)
  3. I end up with 49 shares because orders are truncated to whole numbers

VS:

  1. I have 100 shares
  2. I call order_target(X, 49.9)
  3. I end up with 50 shares because first the difference 100-49.9 = 50.1 is computed, and then 50.1 is truncated to 50, and 100 - 50 = 50.

This is surprising and undesirable -- you should never end up with more shares than you requested, and to the same order_target() call should always result in the same position. I fixed this by truncating the target so now the following happens:

  1. I have 100 shares
  2. I call order_target(X, 49.9)
  3. I end up with 49 shares because first 49.9 is truncated to 49, and only then is the difference 100 - 49 = 51 computed, and 100 - 51 = 49.

When I looked at the existing order_target() unit test, I found that it was only passing by accident. It had two problems. First it compared the position value to the expected value using a price that arrived after the trade had been completed. However this didn't create an issue because it used np.round(), and the position in question was so small that rounding masked the error. Also the quantity being rounded was the position value, not the position size. I rewrote the unit test in this PR and it is passing.

I also added a new unit test to check for order_target behavior when 1) the target is not an integer and 2) the the target is less than the current number of shares. The new test passes with the change to order_target but fails without it.

A big part of this was exposing the int(round_if_near_integer()) logic that the algorithm uses internally to get a round number of shares. I believe that many people have tried to imitate this in their code (I know I have) and I bet there are many ad-hoc versions of it in the unit test (like np.round() in the old order_target() unit tests). These ad-hoc versions probably work most of the time but will sometimes fail -- so I put a module level function called round_shares() in zipline.algorithm so that people don't have to roll their own rounding functions.

@coveralls

This comment has been minimized.

coveralls commented Feb 7, 2015

Coverage Status

Coverage increased (+0.39%) to 87.31% when pulling fbfa555 on jlowin:mod-order_percent into 65c038d on quantopian:master.

@humdings

This comment has been minimized.

humdings commented Feb 8, 2015

I have a question regarding the issue with floats and order_target. Does this come up because order_target_percent and order_target_value can end up passing floats to order_target?

Currently zipline does not support ordering fractional shares, so order_target(stock, 49.9) seems like it shouldn't be a valid input because it's impossible to end up with 49.9 shares. Would it make more sense to make sure the input can be coerced to an int unambiguously rather than come up with a mechanism to round the number of shares to what we think it should be?

@jlowin

This comment has been minimized.

Contributor

jlowin commented Feb 8, 2015

@humdings zipline does coerce all shares to be ints, but it does it at the lowest level in the order() method. I think this is appropriate, because it means people can build on top of order without having to worry about the nuances of how zipline implements its own form of share count rounding. However, in this one case, it was necessary to introduce the rounding at a higher level, in order_target, to avoid the unexpected result. The alternative would be to guard against calling order with int arguments, but I suspect that just creates more work for users with little benefit. Also, as a matter of future-proofing, I think zipline needs to support float share counts. Most brokers won't let traders purchase fractional shares, but they are frequently acquired (and subsequently sold) as a consequence of dividend reinvesting or stock splits.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Feb 9, 2015

@jlowin Travis seems to be complaining:

======================================================================
ERROR: test_order_methods (tests.test_algorithm.TestTransformAlgorithm)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/quantopian/zipline/tests/test_algorithm.py", line 402, in test_order_methods
    algo.run(self.df_2)
  File "/home/travis/build/quantopian/zipline/zipline/algorithm.py", line 498, in run
    for perf in self.gen:
  File "/home/travis/build/quantopian/zipline/zipline/gens/tradesimulation.py", line 129, in transform
    self.algo.instant_fill,
  File "/home/travis/build/quantopian/zipline/zipline/gens/tradesimulation.py", line 234, in _process_snapshot
    new_orders = self._call_handle_data()
  File "/home/travis/build/quantopian/zipline/zipline/gens/tradesimulation.py", line 258, in _call_handle_data
    self.simulation_dt,
  File "/home/travis/build/quantopian/zipline/zipline/utils/events.py", line 194, in handle_data
    event.handle_data(context, data, dt)
  File "/home/travis/build/quantopian/zipline/zipline/utils/events.py", line 212, in handle_data
    self.callback(context, data)
  File "/home/travis/build/quantopian/zipline/zipline/test_algorithms.py", line 442, in handle_data
    self.target_shares[6] += expected_shares(5, half * group_val)
KeyError: 6
-------------------- >> begin captured stdout << ---------------------
Portfolio({'portfolio_value': 99999.699800000002, 'cash': 99979.6998, 'starting_cash': 100000.0, 'returns': -3.0019999999785796e-06, 'capital_used': -20.3002, 'pnl': -0.30019999999785796, 'positions': {0: Position({'amount': 10, 'last_sale_price': 2.0, 'cost_basis': 2.03002, 'sid': 0})}, 'positions_value': 20.0, 'start_date': Timestamp('2006-01-03 00:00:00+0000', tz='UTC', offset='C')})
Portfolio({'portfolio_value': 100009.6998, 'cash': 99979.6998, 'starting_cash': 100000.0, 'returns': 9.6998000000021422e-05, 'capital_used': -20.3002, 'pnl': 9.699800000002142, 'positions': {0: Position({'amount': 10, 'last_sale_price': 3.0, 'cost_basis': 2.03002, 'sid': 0})}, 'positions_value': 30.0, 'start_date': Timestamp('2006-01-03 00:00:00+0000', tz='UTC', offset='C')})
Portfolio({'portfolio_value': 100019.5797744, 'cash': 99995.5797744, 'starting_cash': 100000.0, 'returns': 0.00019579774400001041, 'capital_used': -4.4202256, 'pnl': 19.579774400001043, 'positions': {0: Position({'amount': 6, 'last_sale_price': 4.0, 'cost_basis': 2.03002, 'sid': 0})}, 'positions_value': 24.0, 'start_date': Timestamp('2006-01-03 00:00:00+0000', tz='UTC', offset='C')})```

Can you repro that locally?
Jeremiah Lowin Jeremiah Lowin
@jlowin

This comment has been minimized.

Contributor

jlowin commented Feb 9, 2015

Thanks @twiecki! I forgot to commit a tiny [but critical] change :)

@twiecki twiecki removed the Needs Tests label Feb 9, 2015

elif mv_type == 'ex_cash':
mv = self.portfolio.portfolio_value - self.portfolio.cash
# long invested capital

This comment has been minimized.

@twiecki

twiecki Feb 9, 2015

Contributor

I might be misreading but is this duplicating functionality found here: https://github.com/quantopian/zipline/blob/master/zipline/finance/performance/period.py#L338 ?

This comment has been minimized.

@jlowin

jlowin Feb 9, 2015

Contributor

@twiecki had no idea that was there! I will refactor to reuse the logic where possible.

Jeremiah Lowin added some commits Feb 9, 2015

Jeremiah Lowin Jeremiah Lowin
Jeremiah Lowin Jeremiah Lowin
@coveralls

This comment has been minimized.

coveralls commented Feb 9, 2015

Coverage Status

Coverage increased (+0.44%) to 87.36% when pulling ad1d9f8 on jlowin:mod-order_percent into 65c038d on quantopian:master.

Jeremiah Lowin Jeremiah Lowin
@jlowin

This comment has been minimized.

Contributor

jlowin commented Feb 10, 2015

@twiecki I don't know why the Python2 tests timed out... is it possible to rerun them?

@twiecki

This comment has been minimized.

Contributor

twiecki commented Feb 10, 2015

@jlowin Yeah that happens occasionally. I restarted it.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Feb 12, 2015

Pulled in via dd37a49. Many thanks!

@twiecki twiecki closed this Feb 12, 2015

@twiecki

This comment has been minimized.

Contributor

twiecki commented Feb 18, 2015

@jlowin I had to revert the merge quantopian/qexec@434bda9 as it causec changes to existing algo behavior (more orders being placed). Is this related to the rounding function?

We do want to get this back in but first understand the implications.

@twiecki twiecki reopened this Feb 18, 2015

@coveralls

This comment has been minimized.

coveralls commented Feb 18, 2015

Coverage Status

Coverage increased (+0.36%) to 87.28% when pulling 2de8327 on jlowin:mod-order_percent into 65c038d on quantopian:master.

@freddiev4

This comment has been minimized.

Contributor

freddiev4 commented Apr 2, 2018

Closing this due to inactivity / conflicts. If we still want to add something like this then a new PR would probably be best.

@freddiev4 freddiev4 closed this Apr 2, 2018

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