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

risk rework #2081

Merged
merged 5 commits into from Feb 26, 2018
Merged

risk rework #2081

merged 5 commits into from Feb 26, 2018

Conversation

@llllllllll
Copy link
Contributor

@llllllllll llllllllll commented Jan 17, 2018

posting my current work for feedback

Copy link
Contributor

@prsutherland prsutherland left a comment

I'm not through the whole thing, but sharing comments on parts that I've reviewed.

)

@parameter_space(direction=['long', 'short'])
def test_future_single_position(self, direction):

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Can this test and test_equity_single_position be parameterized/combined? If not a comment to describe the expected differences between the two would be helpful for understanding.

This comment has been minimized.

@llllllllll

llllllllll Jan 24, 2018
Author Contributor

I would prefer not to mix these because the value/exposure calculations are different.

]
if direction == 'long':
all_zero_fields.extend((
'short_value',

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

the *_value are defined above

for asset, shares in iteritems(OrderedDict(zip(
context.assets, {share_counts}
))):
it = zip(context.assets, {share_counts})

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Looks like a lot of this test pre-dates this PR, but it is odd to see the assets hard coded and the share counts dynamic on a template that is only used once.

This comment has been minimized.

@llllllllll

llllllllll Jan 24, 2018
Author Contributor

yeah, these tests are very weird. I am not going to change this now though.

@@ -2454,11 +2431,11 @@ def order_stuff(context, data):
0.0,
0.0,
# 1000 shares * gain of 1
(100000.0 + 1000.0)/100000.0 - 1.0,
(100000.0 + 1000.0) / 100000.0 - 1.0,
# 2000 shares * gain of 1, capital change of +5000

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Comment should read, "... capital change of +50,000"

)

algo.metrics_tracker = algo._create_metrics_tracker()
benchmark_source = algo._create_benchmark_source()
algo.metrics_tracker.handle_start_of_simulation(benchmark_source)

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Does AlgorithmSimulator not call handle_start_of_simulation?

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Ah, I see TradingAlgorithm calls it because it needs the benchmark source. Still, this a method with simulation in the name on object that is largely "owned" by the AlgorithmSimulator. Intuitively, this feels like it should be called by the AlgorithmSimulator.

This comment has been minimized.

@llllllllll

llllllllll Jan 24, 2018
Author Contributor

There is nothing intuitive about the current TradingAlgorithm vs AlgorithmSimulator flow. I will look to see if we can have all of this info somewhere in the tradesim loop.

@@ -1599,34 +1593,39 @@ def order_value(self,
def recorded_vars(self):
return copy(self._recorded_vars)

def _sync_last_sale_prices(self, dt=None):

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

This is sooooo much cleaner than the updated_portfolio stuff before!

@@ -0,0 +1,162 @@
#

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

There appears to be an empty zipline/finance/metrics.py in this pull request that github won't let me comment on. Looks like an accidental commit?

This comment has been minimized.

@llllllllll

llllllllll Jan 24, 2018
Author Contributor

yeah, I will kill that

@@ -970,10 +983,11 @@ def wrapped(*args, **kwargs):
scope = tuple(scope)
try:
f(*args + scope, **kwargs)
except Exception as e:
except Exception:
info = sys.exc_info()

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

What does this change?

This comment has been minimized.

@llllllllll

llllllllll Jan 24, 2018
Author Contributor

Right now, when you use parameter_space or subtest, you don't get the full traceback of each subtest failure. in the past you would set __fail_fast=True to not run subtests and get the traceback; however, that is really annoying. Now, if you have a subtest failure you will see the full traceback (indented) for each parameter set that failed.

asset,
trading_calendar,
trading_days,
data_portal):

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

This is probably in progress as you write more tests, but it looks like the docstring needs the new return type and if self.emission_rate == "minute" the return type is only a Series

This comment has been minimized.

@llllllllll

llllllllll Jan 24, 2018
Author Contributor

good catch

Copy link
Contributor

@prsutherland prsutherland left a comment

At this point I've reviewed everything except zipline/finance/metrics/*

elif field == "contract":
return None
elif field != "last_traded":
return np.NaN

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Looks like this is relocated code, but do you know if "last_traded" should return anything here?

This comment has been minimized.

@llllllllll

llllllllll Jan 31, 2018
Author Contributor

I assume this is not used anymore. ping @ehebert

This comment has been minimized.

@ehebert

ehebert Feb 3, 2018
Contributor

It appears that last_traded is still expected to work. See: https://www.quantopian.com/help#overview-gettingdata

This is the case where the dt is outside of the assets lifetime, where np.NaN was chosen to represent that null value. (However pd.NaT may be more appropriate.)

return stats


if PY2:

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Should this live in compat.py?

This comment has been minimized.

@llllllllll

llllllllll Jan 30, 2018
Author Contributor

we discussed this in person, but this is pretty specialized. we can reconsider relocating this in the future

Parameters
----------
dividends: iterable of (asset, amount, pay_date) namedtuples

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

What do you think of renaming dividends to cash_dividends?

def __init__(self, data_frequency):
# asset => position object
self.positions = OrderedDict()
self._unpaid_dividends = {}

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

I'm guessing the answer is performance/object creation overhead, but why do we not use defaultdicts for the unpaid dividends?

This comment has been minimized.

@llllllllll

llllllllll Jan 30, 2018
Author Contributor

default dicts make it very easy to hide a programming error, setdefault is more explicit

div_owed,
]

def pay_dividends(self, next_trading_day):

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

Should earn_dividends or pay_dividends update self._dirty_stats?

"""

def __init__(self, portfolio):
dict_ = vars(self)

This comment has been minimized.

@prsutherland

prsutherland Jan 24, 2018
Contributor

It is a little confusing that Portfolio uses a MutableView and Account uses a vars dict.

Copy link
Contributor

@prsutherland prsutherland left a comment

We largely discussed this in person yesterday, but writing it down so it doesn't get lost:

We should probably have at least some documentation about how to write/add metrics particularly around how to find what you need on the ledger. If this ends up looking like it will take a non-trivial amount of time, @abhijeetkalyan may have opinions on priority/timing.

Tests to include / scenarios we may want to worry about:

  • Slippage and commission tests (particularly that the new metrics tracker propagates the amount sign consistently with the perf tracker in shorts)

These tests may be exercised elsewhere, but the ledger might be impacted by:

  • Order fills over multiple bars
  • Orders being canceled or not filled by end of day
  • Splits/dividends applied correctly
from zipline.protocol import Portfolio


class PortfolioFieldMetric(object):

This comment has been minimized.

@prsutherland

prsutherland Jan 25, 2018
Contributor

I think this is unused.

This comment has been minimized.

@llllllllll

llllllllll Jan 25, 2018
Author Contributor

I thought I killed this, I replaced it with the ledgerfield stuff since you can get that from ledger.portfolio.*



class AlphaBeta(object):
"""End of simulation alpha and beta to the benchmark.

This comment has been minimized.

@prsutherland

prsutherland Jan 25, 2018
Contributor

The docstring says "end of simulation" but there is no end_of_simulation method.

This comment has been minimized.

@llllllllll

llllllllll Jan 25, 2018
Author Contributor

there may be a few others like this, I will clean these up.

@llllllllll llllllllll force-pushed the risk-rework branch 2 times, most recently from 122ead7 to 5214a71 Jan 30, 2018
@llllllllll llllllllll changed the title (WIP) risk rework risk rework Jan 30, 2018
@coveralls
Copy link

@coveralls coveralls commented Jan 31, 2018

Coverage Status

Coverage decreased (-0.5%) to 87.138% when pulling 5d45c7a on risk-rework into 783cacd on master.

@@ -0,0 +1,274 @@
.. _metrics:

This comment has been minimized.

@prsutherland

prsutherland Feb 2, 2018
Contributor

This is excellent! Very clear!


``next_session_label`` is an :class:`int` which is the index of the next trading
session to be run. This is provided to allow for efficient access to the daily
returns through ``ledger.daily_returns_array[:next_session_ix]``.

This comment has been minimized.

@prsutherland

prsutherland Feb 2, 2018
Contributor

Is the session index continuous so that you can access specific returns by subtracting N days?

This comment has been minimized.

@llllllllll

llllllllll Feb 2, 2018
Author Contributor

yes, the reason it is next_session_ix instead of current_session_ix is to make it easy to do this slice though.

@@ -4491,7 +4471,7 @@ def transactions_for_date(date):
first_auto_close_transaction,
{
'amount': -order_size,
'commission': 0.0,
'commission': None,

This comment has been minimized.

@prsutherland

prsutherland Feb 2, 2018
Contributor

When should commissions be None vs 0.0?

This comment has been minimized.

@llllllllll

llllllllll Feb 2, 2018
Author Contributor

orders have commission, transactions do not.

@@ -198,7 +198,7 @@ def __init__(self,
if reader is not None
]
if last_minutes:
self._last_available_minute = min(last_minutes)
self._last_available_minute = max(last_minutes)

This comment has been minimized.

@prsutherland

prsutherland Feb 2, 2018
Contributor

I would have guessed that this was originally min because that would be the last time we have data for both equity_minute_reader and future_minute_reader. Do we want to run backtests after one reader ends, but while the other still has data?

This comment has been minimized.

@llllllllll

llllllllll Feb 2, 2018
Author Contributor

We already do! On prod right now, you cannot use history after the equities market closes on the current day. imo that should just fill NaN for equities or ffill the price field.

@@ -26,21 +26,43 @@ def sentinel(name, doc=None):
if doc == value.__doc__:
return value

if value._created_at is None:

This comment has been minimized.

@ssanderson

ssanderson Feb 22, 2018
Contributor

Seems like it would be simpler to move this logic down to creation time?

@llllllllll llllllllll force-pushed the risk-rework branch 3 times, most recently from 33b7b3c to b3890d3 Feb 22, 2018
@llllllllll llllllllll force-pushed the risk-rework branch from b3890d3 to 5503af4 Feb 23, 2018
@llllllllll llllllllll force-pushed the risk-rework branch from b600ebe to 5d45c7a Feb 23, 2018
@llllllllll llllllllll merged commit bd24e0d into master Feb 26, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@llllllllll llllllllll deleted the risk-rework branch Feb 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants