Skip to content
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

Supertrend Value Mismatch - proposal to add both implementations #724

Open
MLpranav opened this issue Oct 6, 2023 · 6 comments
Open

Supertrend Value Mismatch - proposal to add both implementations #724

MLpranav opened this issue Oct 6, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@MLpranav
Copy link

MLpranav commented Oct 6, 2023

Which version are you running?
0.3.14b0

Is your feature request related to a problem? Please describe.
Supertrend calculation in pandas-ta is different from most stock/crypto trading websites (like Binance and Zerodha) and platforms, even though many of them are using TradingView's charting library.
On investigating further into this mismatch and checking the source code of various Python and Pinescript implementations of supertrend, it seems like TradingView js charting library's supertrend uses SMA(true_range, length) instead of ATR.

Describe the solution you'd like
I have added a custom indicator named supertrend_classic.
After changing matr = multiplier * atr(high, low, close, length) to matr = multiplier * sma(true_range(high, low, close), length), the results are matching perfectly with the aforementioned platforms' charts.

I'm making this issue to help others with similar problems and also propose that supertrend_classic be included in the main library.

https://www.tradingview.com/script/r6dAP7yi/
Lines 10-11 here illustrate both the approaches.

@MLpranav MLpranav added the enhancement New feature or request label Oct 6, 2023
@MLpranav MLpranav changed the title Supertrend Classic Supertrend Value Mismatch - proposal to add both implementations Oct 6, 2023
@MLpranav
Copy link
Author

MLpranav commented Oct 6, 2023

Alternatively, we can add a simple flag to the existing supertrend indicator so that the user can choose between ATR and SMA(TR) based implementations. Adding a separate indicator for only one minor change will be redundant.

@twopirllc twopirllc removed their assignment Oct 11, 2023
@twopirllc
Copy link
Owner

Hello @MLpranav,

Certainly supertrend v0.3.14b has historical issues. Some of which has been taken care of in the development version.

On investigating further into this mismatch and checking the source code of various Python and Pinescript implementations of supertrend, it seems like TradingView js charting library's supertrend uses SMA(true_range, length) instead of ATR.

According to TV (version 5), it's default is ATR and not SMA(TR). However to include SMA(TR) as an optionable calculation would be trivial include on the development version. Additionally while focus is on updating supertrend it would be great to rewrite it to in numpy/numba to further boost it's calculation speed.

//@version=5
indicator("Pine Script® Supertrend")

[supertrend, direction] = ta.supertrend(3, 10)
plot(direction < 0 ? supertrend : na, "Up direction", color = color.green, style=plot.style_linebr)
plot(direction > 0 ? supertrend : na, "Down direction", color = color.red, style=plot.style_linebr)

// The same on Pine Script®
pine_supertrend(factor, atrPeriod) =>
    src = hl2
    atr = ta.atr(atrPeriod)
    upperBand = src + factor * atr
    lowerBand = src - factor * atr
    prevLowerBand = nz(lowerBand[1])
    prevUpperBand = nz(upperBand[1])

    lowerBand := lowerBand > prevLowerBand or close[1] < prevLowerBand ? lowerBand : prevLowerBand
    upperBand := upperBand < prevUpperBand or close[1] > prevUpperBand ? upperBand : prevUpperBand
    int direction = na
    float superTrend = na
    prevSuperTrend = superTrend[1]
    if na(atr[1])
        direction := 1
    else if prevSuperTrend == prevUpperBand
        direction := close > upperBand ? -1 : 1
    else
        direction := close < lowerBand ? 1 : -1
    superTrend := direction == -1 ? lowerBand : upperBand
    [superTrend, direction]

[Pine_Supertrend, pineDirection] = pine_supertrend(3, 10)
plot(pineDirection < 0 ? Pine_Supertrend : na, "Up direction", color = color.green, style=plot.style_linebr)
plot(pineDirection > 0 ? Pine_Supertrend : na, "Down direction", color = color.red, style=plot.style_linebr)

Lastly, this library primarily covers TA Lib (first) and TV (second). Indicators not covered by TA Lib or TV from other platforms will only be accepted if others are willing to submit PR and provide support for it as needed.

Kind Regards
KJ

@MLpranav
Copy link
Author

MLpranav commented Oct 12, 2023

On digging further, I found that this issue is because different platforms use different moving average types to calculate ATR. Have opened a PR to allow passing the ATR's mamode argument while calling supertrend. Kindly merge @twopirllc.

To illustrate,

Here's one major charting library that uses SMA instead of RMA for ATR calculation:
https://documentation.chartiq.com/tutorial-Using%20and%20Customizing%20Studies%20-%20Definitions.html#atr

And here's a comparison of both the supertrends, yellow one uses RMA for ATR and grey one uses SMA:
image

Additionally while focus is on updating supertrend it would be great to rewrite it to in numpy/numba to further boost it's calculation speed.

Unfortunately I do not have much experience with numba, but I will try to take out some time and address this rewrite in a separate PR.

@twopirllc
Copy link
Owner

@MLpranav

On digging further, I found that this issue is because different platforms use different moving average types to calculate ATR.

Yes and not just ATR. Quite a few indicators have differences in calculations... like TA Lib's EMA comment.

   /* The first EMA is calculated differently. It
    * then become the seed for subsequent EMA.
    *
    * The algorithm for this seed vary widely.
    * Only 3 are implemented here:
    *
    * TA_MA_CLASSIC:
    *    Use a simple MA of the first 'period'.
    *    This is the approach most widely documented.
    *
    * TA_MA_METASTOCK:
    *    Use first price bar value as a seed
    *    from the begining of all the available
    *    data.
    *
    * TA_MA_TRADESTATION:
    *    Use 4th price bar as a seed, except when
    *    period is 1 who use 2th price bar or something
    *    like that... (not an obvious one...).
    */

Unfortunately I do not have much experience with numba, but I will try to take out some time and address this rewrite in a separate PR.

Converting supertrend's for loop into a numpy/numba for alphatrend would be similar if you need an example. There is no urgency.

KJ

@krazykoder
Copy link

@twopirllc - I will try to create a numba version and post here. But I don't have experience in bench marking testing it with this lib; if you can take over from there, appreciate it.

@twopirllc
Copy link
Owner

@krazykoder

Cool! 😎

No worries about testing. But if you are curious, there is a Speed Test notebook you can run to check the performances when you are satisfied with your code.

KJ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants