mdeverdelhan/ta4j-origins

ParabolicSAR incorrect values

mattgreenuk opened this issue · 7 comments

mattgreenuk commented

Firstly thanks for the great lib!
I've been integrating ta4j into my trading pipeline, I'm calculating Parabolic SARs based on 1-minute ticks and have been testing using real data. There definitely appears to be a discrepancy between the ta4j calculated Parabolic SAR and two other sources.
I've run the same data (1 hour of the FTSE100 index) through the following:

Here is a graph showing the 3 SARs compared to open and close prices for each minute:
image

Things of particular note from the graph that may help investigation (or ease understanding of my rough and ready analysis!):

  • The spreadsheet values don't start until time period 4
  • The spreadsheet and IG Index values correlate except for a 1 period lag, this is probably due to them calculating at the beginning or end of a time period - either way, they're calculating the same values
  • ta4j values seem too close to the actual prices - this perhaps indicates an issue with the "extreme point" value which is part of the SAR calculation. It also explains the trend flip-flop that occurs with ta4j - the prices touch the SAR value too easily.
  • IG Index was calculating the SAR for earlier periods than shown on the graph hence their data doesn't suffer from the 'usual' start-up "problems" that this algorithm exhibits

The same values occur with version 0.8 and version 0.9-SNAPSHOT and the latest code from github.
I've created a fork and added some additional test cases into ParabolicSARTest.java including some CSV files with the raw prices and expected SARs from the two reference sources.

Here is a PR containing those changes, apologies that they're a bit crude: #146

I'll continue to investigate...

mdeverdelhan commented

Thank you for such a detailed issue. I already saw that Parabolic SAR was behaving weirdly sometimes. I reviewed the code but did not find anything that would have been suspect.
How are your investigations?

Advice: when testing, change the extends RecursiveCachedIndicator<Decimal> by extends Indicator<Decimal> (Here: https://github.com/mdeverdelhan/ta4j/blob/master/ta4j/src/main/java/eu/verdelhan/ta4j/indicators/trackers/ParabolicSarIndicator.java#L38), you may get some clues.

karvanit commented

It seems to me that the way this implementantion decides to switch is not correct. It should detect uptrend / downtrend when the calculated SAR is within the current [Low, High] range. At least that's my understanding of the article in wikipedia

whitfa99 commented

Maybe this existing code:

    if (n2ClosePrice.isGreaterThan(n1ClosePrice) && n1ClosePrice.isLessThan(nClosePrice)) {
    // Trend switch: \_/

..should instead be something like (warning: pseudo-code!):

    if (series.getTick(index).getMinPrice().isLessThan(sar.previous.getvalue) && sar.currentrend==up) {
    // Trend switch: \_/

And maybe the (int timeFrame) main argument is a bit redundant somehow if the Extreme point is just moved based if there's a new high on an uptrend (or new low for downtrend)? But maybe I've misunderstood how this is used.

Sorry, I'm still grappling with java and can't quite work out how to change the code..

team172011 commented

We are going to continue this bug in the successor repository

mdeverdelhan commented

Congratulations @team172011 !

team172011 commented

Thank you!