Skip to content
This repository has been archived by the owner on Sep 2, 2022. It is now read-only.

added supertrend indicator #104

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

kitt-th
Copy link

@kitt-th kitt-th commented Dec 4, 2020

Hi @peerchemist , I added the supertrend indicator. I cross checked results with the original pinescript indicator linked in the indicator description.

black also reformatted some of the comments/lines in finta.py

new code is on lines 1408-1466

this is my first git pull request, so I hope I did it right. let me know what you think!

@peerchemist
Copy link
Owner

Thanks for the contribution. Unfortunately I will not be able to review before Monday.

@kitt-th
Copy link
Author

kitt-th commented Dec 5, 2020 via email

@peerchemist
Copy link
Owner

Please add unit test.

@kitt-th
Copy link
Author

kitt-th commented Dec 7, 2020

NP will get to that. and submit.

Copy link

@arul67800 arul67800 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This formula has errors.. you cannot start the range from 1, since st_data values start after atr period+ supertrend period

@kitt-th
Copy link
Author

kitt-th commented Dec 14, 2020

@arul67800 - thanks for pointing that out. The range should start at 'period +1' as supertrend calculations reference the value of the previous time series/candle. Updated commit coming next.

@peerchemist
Copy link
Owner

Hi, you are trying to push some weird dotfiles in the tests directory.

tests/data/.DS_Store

@kitt-th
Copy link
Author

kitt-th commented Dec 16, 2020

Hi, sorry about that - my first pull requests so I will check all the files again, clean up and commit without those extra things. Thanks for your patience. I must have made a mistake when adding files.

… that the finder adds. Used gitignore to filter them out
@ph4z
Copy link

ph4z commented Feb 1, 2021

Hi,
@peerchemist It seems the PR has been cleaned by @kitt-th
I would like to test this indicator, so it would be nice if you can merge it to the master.
By the way I would be very intrested by the 'megatrend' indicator. Unfortunately I couldn't find the or source/formula for it...

@peerchemist
Copy link
Owner

Unfortunately this indicator did not work for me in my tests, and I did not have time in a good while to play and hack on it.

@kitt-th
Copy link
Author

kitt-th commented Feb 5, 2021

Did it not work for you with results not being as expected or was there some other error? @peerchemist

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants