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

Move risk calculations #1322

Merged
merged 4 commits into from Aug 23, 2016

Conversation

Projects
None yet
9 participants
@ahgnaw
Contributor

ahgnaw commented Jul 13, 2016

Centralize the way metrics are calculated in zipline by adding qrisk as a dependency and replacing the current functions.

Testing of actual calculated values within classes has been removed and replaced by a test of existence and type. Any test for metric correctness will be done in the qrisk repository from now on.

Treasury curves are no longer used as the risk-free rate when calculating the sharpe ratio.

@jbredeche

This comment has been minimized.

Member

jbredeche commented Jul 13, 2016

sweet!

@@ -1,87 +0,0 @@
{

This comment has been minimized.

@ehebert

ehebert Jul 13, 2016

Member

👍

I can not recall if this powers a link on the forums discussing risk metrics.
I think it is ok to remove, since if that link does break, we can change the link to point to a commitish where this file does exist (if it doesn't already.)

end_date = datetime.datetime(
year=2006, month=12, day=31, tzinfo=pytz.utc)
self.start_date = datetime.datetime(2006, 1, 1, tzinfo=pytz.utc)

This comment has been minimized.

@ehebert

ehebert Jul 13, 2016

Member

Since this test had been written, we have more or less standardized on using pd.Timestamp instead of datetime.datetime.

@abhijeetkalyan

This comment has been minimized.

Member

abhijeetkalyan commented Jul 13, 2016

@ahgnaw @ehebert what do you think of keeping the existing methods in places like risk.py, but just making them forward directly to the qrisk functions? This is similar to what we did in pyfolio, where we kept the existing functions and made them forward directly to qrisk. To make it clear we'd change this in the future, we added deprecation warnings.

@@ -62,3 +62,6 @@ sortedcontainers==1.4.4
intervaltree==2.1.0
cachetools==1.1.5
# For financial risk calculations
qrisk>=0.1.4

This comment has been minimized.

@richafrank

richafrank Jul 13, 2016

Member

We should peg the version to exactly 0.1.4. Currently we use this like a constraints file for our environments, and setup.py does some replacement to make pip installing zipline require >= the versions specified here.

self.benchmark_returns)
def calculate_beta(self):
def calculate_covariance(self):

This comment has been minimized.

@ehebert

ehebert Jul 13, 2016

Member

I believe that we were only calculating covariance as an input to pass into the beta calculation.
With pyrisk now handling the calculation, we may be able to remove the covariance calculation from Zipline completely.

(I checked the to_dict methods in risk.cumulative and risk.period and it does not appear there, meaning it is not exposed in risk packets.)

@ehebert

This comment has been minimized.

Member

ehebert commented Jul 13, 2016

@abhijeetkalyan risk.py was intended to share common functions between period.py and cumulative.py. An advantage of the current PR is fewer moving pieces, with qrisk serving the role formerly done by risk.py.

@ehebert

This comment has been minimized.

Member

ehebert commented Jul 13, 2016

@ahgnaw this looks good to me. (Glad to have this old code replaced!)

Two notes:

  • Looks like some stray from . import answer_key are still present, breaking the build.
  • Left a comment about removing covariance.
@@ -0,0 +1,27 @@
package:

This comment has been minimized.

@llllllllll

llllllllll Jul 13, 2016

Member

this should probably live in the qrisk package.

ehebert added a commit that referenced this pull request Jul 14, 2016

len(self.algo_returns),
len(self.cumulative_metrics.algorithm_volatility)
)
np.testing.assert_equal(

This comment has been minimized.

@ssanderson

ssanderson Jul 18, 2016

Member

If all we want to test is that an array contains floats, the right thing to do is assert array.dtype == np.dtype('float64'). Separately, is this test even worth having if it's making no assertions about the output content? Ditto most of the rest of the tests in this suite.

This comment has been minimized.

@ahgnaw

ahgnaw Jul 18, 2016

Contributor

The output content is being tested in qrisk. We could have multiple locations that test the correctness of the underlying function, but that might get confusing down the line. We want to still test the class properties.

len(self.algo_returns),
len(self.cumulative_metrics.downside_risk)
)
np.testing.assert_equal(

This comment has been minimized.

@ssanderson

ssanderson Jul 18, 2016

Member

assert_equal(expr, True) is just assert expr.

@ahgnaw ahgnaw force-pushed the move_risk_calculations branch 11 times, most recently from 9bdd2dc to a1e9883 Jul 18, 2016

@twiecki twiecki referenced this pull request Jul 27, 2016

Closed

Fix sharpe ratio #877

@twiecki

This comment has been minimized.

Contributor

twiecki commented Jul 28, 2016

This is my favorite PR.

@ahgnaw ahgnaw force-pushed the move_risk_calculations branch 2 times, most recently from e67fead to f6f8398 Aug 11, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 18, 2016

Coverage Status

Coverage increased (+0.02%) to 85.959% when pulling 462eae7 on move_risk_calculations into a1b48c4 on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 18, 2016

Coverage Status

Coverage increased (+0.02%) to 85.959% when pulling 462eae7 on move_risk_calculations into a1b48c4 on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 18, 2016

Coverage Status

Coverage increased (+0.02%) to 85.959% when pulling 462eae7 on move_risk_calculations into a1b48c4 on master.

2 similar comments
@coveralls

This comment has been minimized.

coveralls commented Aug 18, 2016

Coverage Status

Coverage increased (+0.02%) to 85.959% when pulling 462eae7 on move_risk_calculations into a1b48c4 on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 18, 2016

Coverage Status

Coverage increased (+0.02%) to 85.959% when pulling 462eae7 on move_risk_calculations into a1b48c4 on master.

@ahgnaw ahgnaw force-pushed the move_risk_calculations branch from 462eae7 to fcf0357 Aug 23, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage increased (+0.09%) to 86.285% when pulling fcf0357 on move_risk_calculations into b9a7e2f on master.

Ana Ruelas added some commits Jun 22, 2016

Ana Ruelas
ENH: Use qrisk to calculate risk metrics in cumulative and period
TST: Remove metric correctness testing from period and cumulative tests

ENH: Removed answer key and related files

ENH: Update qrisk version
Ana Ruelas
TST: Update to empyrical, increase test coverage
ENH: Resolve rebase conflict by using updated example_data.tar

TST: Increase test coverage for risk portion of zipline

@ahgnaw ahgnaw force-pushed the move_risk_calculations branch from fcf0357 to 7f04247 Aug 23, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage increased (+0.02%) to 86.216% when pulling 7f04247 on move_risk_calculations into 82436aa on master.

@richafrank

This comment has been minimized.

Member

richafrank commented Aug 23, 2016

@ahgnaw Would you add a bullet to the latest whatsnew that describes this change?

@ahgnaw

This comment has been minimized.

Contributor

ahgnaw commented Aug 23, 2016

@richafrank Can do

@@ -36,6 +36,12 @@ Enhancements
produced a True on N or more days in the previous ``window_length``
days (:issue:`1367`).
- Remove risk-free rate from sharpe ratio calculation. (:issue:`853`).

This comment has been minimized.

@richafrank

richafrank Aug 23, 2016

Member

Seems like this fixed other issues with the sharpe ratio (mean versus cumulative returns, etc)? Should we mention those and/or move this to Bug Fixes?

This comment has been minimized.

@ahgnaw

ahgnaw Aug 23, 2016

Contributor

Good point. I'll include changes to other metrics as well.

- Use external library empyrical for risk calculations. Empyrical unifies risk
metric calculations between pyfolio and zipline. Empyrical adds custom
annualization options for returns of custom frequencies.

This comment has been minimized.

@richafrank

richafrank Aug 23, 2016

Member

Looks like a good place to reference #855.

@@ -62,3 +62,6 @@ sortedcontainers==1.4.4
intervaltree==2.1.0
cachetools==1.1.5
# For financial risk calculations
empyrical>=0.1.9

This comment has been minimized.

@richafrank

richafrank Aug 23, 2016

Member

Can we pin this to an exact version, like all the others?

This comment has been minimized.

@richafrank

richafrank Aug 23, 2016

Member

Our setup.py should already convert this to a >=, so you can pip install zipline with a newer version of empyrical.

@ahgnaw ahgnaw force-pushed the move_risk_calculations branch 3 times, most recently from 5c08cfa to 99bc7da Aug 23, 2016

@ahgnaw ahgnaw force-pushed the move_risk_calculations branch from 99bc7da to 5b0ff75 Aug 23, 2016

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage decreased (-0.4%) to 85.813% when pulling 5b0ff75 on move_risk_calculations into 82436aa on master.

@coveralls

This comment has been minimized.

coveralls commented Aug 23, 2016

Coverage Status

Coverage increased (+0.02%) to 86.216% when pulling 5b0ff75 on move_risk_calculations into 82436aa on master.

@ahgnaw ahgnaw merged commit eba78ae into master Aug 23, 2016

2 checks passed

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

@ahgnaw ahgnaw deleted the move_risk_calculations branch Aug 23, 2016

bartosh pushed a commit to bartosh/zipline that referenced this pull request Sep 27, 2016

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