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

Momentum Indicators - RSI, MACD ... (#120) #131

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

fmilthaler
Copy link
Owner

@fmilthaler fmilthaler commented Aug 5, 2023

Implementation of RSI momentum indicator, thank you @pythonhacker. This closes #119.

@fmilthaler fmilthaler marked this pull request as draft August 5, 2023 15:53
@pythonhacker
Copy link

Hi @fmilthaler - Any updates on if/when you may merge this ? Thank you.

@fmilthaler
Copy link
Owner Author

fmilthaler commented Sep 9, 2023

Hey, sorry for the delay, I was away and then busy otherwise.
Anyway, I found several (different) implementations of the RSI, and they all differ from another. I still want to go through them and understand them better and see what can be used here. Funny thing is, they give different results. As for the macd, it might be good to use the example of https://github.com/matplotlib/mplfinance/blob/master/examples/indicators/macd_histogram_gradient.ipynb now rather in a later merge.

@pythonhacker
Copy link

Do you want me to modify the code ?

@fmilthaler
Copy link
Owner Author

fmilthaler commented Sep 22, 2023

Sorry for the delay @pythonhacker. I had to have a closer look at the RSI itself to understand it first.

As for the implementation of the rsi relative_strength_index, can you please remove the for loops and use vectorized computations instead for the sake of performance and readability?
Note: I just realized the dependecy within the for loop, thus a vectorized computation is not possible here. Too bad :(

For the macd, could you make use of the package mplfinance here instead? I don't like reinventing the wheel here. We can simply use one of the following:

to plot the macd.

Again, sorry for the delay in this, I've been busy and pushed this back too many times.

@fmilthaler
Copy link
Owner Author

fmilthaler commented Oct 1, 2023

@pythonhacker I've updated the code to be compliant with the recent master and made changes to the macd function to be like the example in mplfinance.
Have a look at everything and let me know if you are happy with that. If so, we can then get it checked by Pietropaolo and then merged.

Note:
Mypy is not happy yet, thus more changes are required. If you want to have a look at that, please run scripts/run_code_analysis.sh and go through the mypy issues and try to fix them.

This is what the MACD plot looks like now:
image

@fmilthaler fmilthaler marked this pull request as ready for review October 1, 2023 14:52
@fmilthaler fmilthaler marked this pull request as draft October 2, 2023 13:51
@fmilthaler fmilthaler mentioned this pull request Dec 3, 2023
@pythonhacker
Copy link

@fmilthaler - I have been out of touch for a while, due to work and just looked back at this. Didn't see the review request my bad. Kindly let me know if I can still review this.

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

Successfully merging this pull request may close these issues.

Momentum Indicators - RSI, MACD
2 participants