search

peerchemist / finta

Common financial technical indicators implemented in Pandas.
GNU Lesser General Public License v3.0
2.13k stars 688 forks source link

added supertrend indicator #104

Open kitt-th opened 3 years ago

kitt-th commented 3 years ago

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 commented 3 years ago

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

kitt-th commented 3 years ago

no problem!

On Fri, 4 Dec 2020 at 16:25, peerchemist notifications@github.com wrote:

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

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/peerchemist/finta/pull/104#issuecomment-738844465, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJWB6LVMI5SG4KUZYMWENTDSTD5MXANCNFSM4UNFIBJQ .

peerchemist commented 3 years ago

Please add unit test.

kitt-th commented 3 years ago

NP will get to that. and submit.

kitt-th commented 3 years ago

@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 commented 3 years ago

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

tests/data/.DS_Store

kitt-th commented 3 years ago

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.

ph4z commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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