Sharpe ratio is calculated incorrectly

[benmccann]

The Sharpe ratio is being calculated incorrectly. It should be using the mean of the difference between the portfolio return and benchmark return and not the difference of the cumulative returns.

[ehebert]

You're right, we should do the mean difference.

The recent efforts with adding the answer key (https://github.com/quantopian/zipline/blob/master/tests/risk/answer_key.py) are mainly in service of fixing this bug in particular.

Currently, the sharpe method just takes in the scalar value of the current return for both algorithm and treasury, instead of a return vector, so some rewiring needs to be done both in the risk module and the spreadsheet.

One thing I am investigating, is whether the current behavior is possibly correct for the fixed sized rollups or not, or whether we should always be using the mean.

Either way for above question, the end result/headline sharpe needs to change.

[briancappello]

Looking further into this, I'm getting thrown off by the * sqrt(trading_days) portion of the calculate_volatility function. It seems to me that's de-averaging the standard deviation calculation? (The denominator of Sharpe) Perhaps it depends on which version of Sharpe you're referencing?

I'm also curious if there's any interest in providing a HFT Sharpe variant too? That would be much more useful for short-term algos. (eg calculated without the "risk-free" benchmark, which is meant to compensate for opportunity costs and overnight holding risk) Skewness and Kurtosis measures might also be helpful for analyzing the distributions of returns, especially when they are fat-tailed.

[ehebert]

@briancappello the sqrt(trading_days) is intended only for the fixed length roll up periods, however there is a bug/design problem where trading days is being used for periods greater than one year, which should be using sqrt(252) instead, so that strategies can be compared independent of time range.

And, a more intraday friendly ratio would be welcomed, I believe.

[twiecki]

We already have annualizer in TradingAlgorithm in anticipation of fixing this. I think we just need to change it to sqrt(algo.annualizer).

[ehebert]

This had been fixed in master for a while, finally released with release v0.6.0