askmike / gekko

A bitcoin trading bot written in node - https://gekko.wizb.it/
MIT License
10.07k stars 3.94k forks source link

[Backtesting] Sharpe ratio calculation is incorrect #2064

Closed zappra closed 6 years ago

zappra commented 6 years ago

Note: this is the technical bug tracker, please use other platforms for getting support and starting a (non technical) discussion. See the getting help page for details.

I'm submitting a ... [x ] bug report [ ] question about the decisions made in the repository

Action taken (what you did)

Backtesting via UI

Expected result (what you hoped would happen)

Ex-post Sharpe ratio should calculate the mean of P(t) - RFR(t) for all roundtrips, where P(t) is the profit for the roundtrip, and RFR(t) is the risk free return for the period of the roundtrip

Actual result (unexpected outcome)

The numerator is calculated as the mean of all roundtrip profits, less an annual risk free return rate. This is incorrect, and will more often than not yield incorrect negative Sharpe ratios, even if on average the strategy is profitable.

Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix, links for us to have context, eg. stackoverflow, etc)

Also, might the Sortino ratio be a better fit for Crypto? The Sortino denominator is the stdDev of losses only; in other words, the volatility of positive returns doesn't punish the ratio.

stereohelix commented 6 years ago

Hi zappra,

I'm wondering if this will help or not:

  1. Since Sharpe usually refers to an annual risk/return, shouldn't it be calculated relativeYearlyProfit? Something like this:

this.sharpe = (relativeYearlyProfit - perfConfig.riskFreeReturn) / statslite.stdev(this.roundTrips.map(r => r.profit));

  1. Since we are deriving standard deviation from a sample, I think it should have a Bessel correction:
    this.sharpe = (relativeYearlyProfit - perfConfig.riskFreeReturn) 
    / statslite.stdev(this.roundTrips.map(r => r.profit)) 
    / Math.sqrt(this.trades / (this.trades - 2));

(where each sample is 1 roundtrip or 2 trades, hence the statistical correction is 2 trades)

  1. This is probably a little more controversial, but the annual profit could be compounded every period -- it's a little more accurate in some sense but could also exaggerate the consistency of returns. It would look something like:
  let annualCompound = this.round(1 / timespan.asYears());
  let relativeYearlyProfit = Math.pow(balance / this.start.balance, annualCompound) * 100 - 100;
  let yearlyProfit = relativeYearlyProfit / 100 * this.start.balance;

Just some ideas, hope this helps.

zappra commented 6 years ago
  1. Yes that makes sense, I prefer this to my version

  2. Never heard of a Bessel correction but I'll take your word for it!

  3. I think your first idea is a better compromise.

For me personally, the ratio would encapsulate a few things:

Profitability (obvs) Volatility of downside risk, and/or max drawdown, and/or max number of consecutive loss making trades Trade frequency - I would prefer a strategy that made regular trades, because I personally consider scalping off volatility as less risky than holding for the long term.

However thats all quite personal and others might favour a different bias, so perhaps there should be configuration options?

I'd also vote to rename it to "Gekko Ratio" - since a) it might end up being a hybrid of other ratios and b) it's primary use would be to compare one Gekko strategy against another, or for genetic backtesting purposes. Anyone using it to compare against traditional investments needs locking up! (IMHO, obvs, etc)

stereohelix commented 6 years ago

You could also do a separate ratio or calculation for downside trades. Dividing by the standard deviation would be a little deceptive (you'd get lower negative values with low deviations) so maybe taking a lower quantile instead:

in roundtrips:

if (roundtrip.exitBalance < roundtrip.entryBalance)
  this.roundTrips.losses.push(roundtrip);

then

let downsideQuartile = statslite.percentage(this.roundTrips.losses.map(r => r.profit), 0.25);

I'm not sure if that's right, this is just off the cuff.

askmike commented 6 years ago

fixed in #2178. Due to a big internal upgrade unfortunately this fix will only land in stable after #1850 is merged.