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

Fix Sharpe ratio calculation #853

Closed
twiecki opened this Issue Nov 17, 2015 · 12 comments

Comments

Projects
None yet
7 participants

@twiecki twiecki added the Bug label Nov 17, 2015

@dunster

This comment has been minimized.

dunster commented Nov 17, 2015

Does this mean the "answer key" is also wrong?

@twiecki

This comment has been minimized.

Contributor

twiecki commented Nov 17, 2015

I'm not sure, I find it hard to navigate that. @justinlent could you help taking a look at what's going on there?

@justinlent

This comment has been minimized.

justinlent commented Nov 17, 2015

Sure. Maybe I can take the answer key's daily returns and import them into pyfolio to try to figure it out

@justinlent

This comment has been minimized.

justinlent commented Nov 17, 2015

@dunster @twiecki I've done a deep dive into the answer key document. It looks like there are some issues as I look closely into the formulas and their interconnectedness. There is not consistency between using arithmetic and compounded returns which is causing imprecise calculation of Sharpe. I've added new columns to the answer key which I will share with Jess and Thomas, and whomever else would like to provide a fresh set of eyes to review my modifications. Once all are in agreement we can propagate the changes to zipline and pyfolio (only 1 minor change to pyfolio, more to zipline it seems)

@llllllllll

This comment has been minimized.

Member

llllllllll commented Nov 18, 2015

I had a conversation a couple of days ago with Jess about the risk metrics in zipline vs pyfolio. This was brought up because of the issue with information ratio that we recently found. What I would really like to do is move all of the risk metric calculations to a new project. Both pyfolio and zipline would consume this project for the risk metrics. This would mean making the risk free rate a parameter to some of these functions where I belive pyfolio currently uses 0 implicitly.

The main reason for moving this to a new repo would be to make all the risk metrics that we report consistent. We can share implementation and tests for zipline and pyfolio which is great for avoiding duplication and introducing subtle differences. I would want this to be a standalone project because I don't think it makes sense for zipline to be a dependency of pyfolio or for pyfolio to be a dependency of zipline.

@justinlent

This comment has been minimized.

justinlent commented Nov 18, 2015

@llllllllll This sounds great to me. Do you propose us waiting to migrate to the soon-to-be-developed risk module without fixing the existing bugs in zipline? I think we should at least fix the Sharpe Ratio (and the underlying annual returns intermediate calculation), unless you think migrating to the to-be-developed module won't take that long. How long do you think it would take?

@llllllllll

This comment has been minimized.

Member

llllllllll commented Nov 18, 2015

I am not sure. I don't know who will own making this change but I imagine in will need to be a joint effort from the pyfolio and zipline teams. @dunster and @richafrank, any thoughts on this?

@twiecki

This comment has been minimized.

Contributor

twiecki commented Nov 18, 2015

Lets move the discussion about risk-metrics to #855 and keep this to fixing Sharpe.

@twiecki

This comment has been minimized.

Contributor

twiecki commented Nov 18, 2015

We should definitely fix it in zipline first. Sharpe is a critical statistic we use throughout. The fix also shouldn't be too difficult.

@dunster

This comment has been minimized.

dunster commented Nov 18, 2015

I think it would be a good idea to show @ehebert because he did a lot of work on the answer key in the past. Definitely @Jstauth too.

@ehebert

This comment has been minimized.

Member

ehebert commented Nov 23, 2015

@twiecki your changes seem reasonable from an implementation stand point (and I defer to your and @justinlent's expertise on the why).

If the spreadsheet is too difficult work with, it may have outlived its usefulness, especially know that we have more experts looking at the risk calculations.
While making this fix, we could deprecate its usage (at least for Sharpe) and have the Sharpe tests in tests.risk.test_risk_cumalitive be checked with input defined in Python in the test suite instead of checking the answer sheet.

twiecki added a commit that referenced this issue Nov 25, 2015

BUG Fix Sharpe ratio calculation.
See #853 for more details.

twiecki added a commit that referenced this issue Nov 25, 2015

BUG Fix Sharpe ratio calculation.
See #853 for more details.

twiecki added a commit that referenced this issue Nov 26, 2015

BUG Fix Sharpe ratio calculation.
See #853 for more details.

twiecki added a commit that referenced this issue Dec 1, 2015

BUG Fix Sharpe ratio calculation.
See #853 for more details.

twiecki added a commit that referenced this issue Dec 2, 2015

@ahgnaw

This comment has been minimized.

Contributor

ahgnaw commented Aug 23, 2016

@ahgnaw ahgnaw closed this Aug 23, 2016

@ahgnaw ahgnaw reopened this Aug 24, 2016

@richafrank richafrank closed this Sep 9, 2016

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