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

Add True Momentum Oscillator (TMO) #729

Merged
merged 1 commit into from
Nov 14, 2023

Conversation

luisbarrancos
Copy link
Contributor

@luisbarrancos luisbarrancos commented Oct 13, 2023

Added TMO indicator, addressing issue #700. defaults 14/5/3 values. See documentation for the calculation, defaults, and variants presented by some common platforms, also listed in the references.

Added example jupyterlab notebook showing how we can trivially compute the necessary information to replicate the expected results from other platforms. Added as well the respective plots with matplotlib, plotly, bokeh, with conditional shading.

@luisbarrancos
Copy link
Contributor Author

luisbarrancos commented Oct 13, 2023

@twopirllc can you please confirm that in tests_studies.py:12 one should add the columns a new indicator introduces? Is this to be accounted for and added as well in test_studies.py:33 by adding the N columns your indicator introduces, into the momentum category?
An indicator without a TA-lib implementation requires any specific setting?

Thanks in advance.

Added TMO indicator, defaults 14/5/3 values. See documentation for the
calculation, defaults, and variants presented by some common platforms.

Added example jupyterlab notebook showing how with the present
information is trivial to compute the variants that other platforms
present.

Added plots with matplotlib, plotly, bokeh, with conditional shading.
@twopirllc
Copy link
Owner

@luisbarrancos

Good questions. Was the quickest solution I could come up with without trying to dynamically figure it out. 😅

can you please confirm that in tests_studies.py:12 one should add the columns a new indicator introduces?

Yes

Is this to be accounted for and added as well in test_studies.py:33 by adding the N columns your indicator introduces, into the momentum category?

Yes

An indicator without a TA-lib implementation requires any specific setting?

I don't recall off the top of my head. But my gut says no.

Thanks for the help! 😎

@luisbarrancos
Copy link
Contributor Author

luisbarrancos commented Oct 14, 2023

@twopirllc thank you, i guess we're good to go with this PR, unless anything missed my attention.
Regarding the implementation, there's a faster way to do it with numpy and numba, where we take advantage that this indicator is built from a moving average of a summation. Since we can see the summation as a convolution with a kernel of all 1s with width L, and a MA is also a convolution, due to linearity and associativity we have $$x[n] = \left(x[n] \ast k_1\right) \ast k_2 = x[n] \ast \left(k_1 \ast k_2\right) = x[n] \ast k_3$$ where $k_3$ is the kernel you obtain by convolving the summation and the MA kernel, with respective width $n_1 + n_2 - 1$ for the full convolution case and $n_1 - n_2 + 1$ case for the valid convolution case, IIRC:

Tests with numba were matching the Pandas and Numpy approach, with slight numerical precision issues i believe. This warrants further tests and timings. If you're heading in that direction let me know and i can submit another PR later with a faster numpy+numba+combined convolution approach.

@twopirllc
Copy link
Owner

@luisbarrancos

Regarding the implementation, there's a faster way to do it with numpy and numba ...

I would prefer to take care of the numpy/numba version soon than later. Also a later PR is totally fine. No rush. 😎

@luisbarrancos
Copy link
Contributor Author

luisbarrancos commented Oct 15, 2023

@luisbarrancos

Regarding the implementation, there's a faster way to do it with numpy and numba ...

I would prefer to take care of the numpy/numba version soon than later. Also a later PR is totally fine. No rush. 😎

@twopirllc Ok. Feel free to merge this at will then. Performance wise it's not bad, since the moving averages are already optimized and using TA-lib if available as well. I'll investigate the numerical issues with the combined kernel approach, but have in mind that by virtue of allowing the user to specify the moving average type, we might need to combine the summation with all the alternative MAs. This might pose a problem if you have piecewise functions, since you would end up with non-standard convolutions, and the cost benefit ratio might not justify the investment.
For the odd indicator that starts from moving averages of rolling sums, it might be worthwhile.
I'll be doing this out of personal curiosity and will publish the work in progress, timing results, tests, in a ticket with associated jupyterlab notebooks, and we can decide from the ticket how to proceed, and if it's worth it.

twopirllc added a commit that referenced this pull request Nov 14, 2023
@twopirllc twopirllc merged commit 274efc0 into twopirllc:development Nov 14, 2023
1 check passed
@twopirllc
Copy link
Owner

Hey @luisbarrancos,

Thanks again for contribution and patience to the library! 😎

I did some refactoring here and there and modified the columns names slightly. When you get a chance, can you give it spin to make sure it works as intended after my adjustments.

Thanks,
KJ

@luisbarrancos
Copy link
Contributor Author

@twopirllc no worries really. It's a perfect excuse to spend more time with stochastic calculus and signal processing.
I tried running the notebooks, everything seems to be ok. From what i could gleam, the missing issues from my side in the PR were updating the README.md file, adding the new indicator to the momentum section, and convention issues for variable names and dataframe fields. I'll have that in mind for subsequent pull requests.

As discussed earlier, i'm investigating the possibility of optimizing this further by combining the sum and initial MA kernels, which might be beneficial if you have particularly large time series to process, but this will be for another PR. Thanks for the PR review(s) and suggestions.

@twopirllc
Copy link
Owner

@luisbarrancos

no worries really. It's a perfect excuse to spend more time with stochastic calculus and signal processing.

Yeah, those are interesting fields. 😎

I tried running the notebooks, everything seems to be ok.

Awesome. I avoided substituting calculation alternatives at this time. It's not slow on my end.

From what i could gleam, the missing issues from my side in the PR were updating the README.md file, adding the new indicator to the momentum section, and convention issues for variable names and dataframe fields. I'll have that in mind for subsequent pull requests.

Don't worry about the stuff you missed too much. Especially variable names and resultant df column names. Sometimes naming them is tricky (even for me).

As discussed earlier, i'm investigating the possibility of optimizing this further by combining the sum and initial MA kernels, which might be beneficial if you have particularly large time series to process, but this will be for another PR. Thanks for the PR review(s) and suggestions.

Sounds great! Looking forward to it. I appreciate all your help. 😎

Thanx
KJ

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.

2 participants