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

Ichimoku indicators are not completely correct #452

Closed
Thomas-Heniart opened this issue Apr 15, 2024 · 10 comments · Fixed by #455 or #459
Closed

Ichimoku indicators are not completely correct #452

Thomas-Heniart opened this issue Apr 15, 2024 · 10 comments · Fixed by #455 or #459

Comments

@Thomas-Heniart
Copy link
Contributor

Describe the bug
The Tenkan-Sen should be based on the last 9 periods but the first 8 values are incorrect.
For example, the first value is calculated as follows: (highs[0] + lows[0]) / 2
The same goes for the Kijun-Sen, SSA, and SSB.

Also, SSA and SSB are incorrect for another reason too. They should be moved 26 periods in the future. By definition, SSA and SSB arrays should have 26 more indexes than Tenkan-Sen, Kijun-Sen, and Chikou-Span.

I think there is also another issue with the Chikou-Span (lagginsSpan) so I may update this issue a bit later.

Expected behavior
Until we have enough periods, the value should be null or any other default value.

@cinar
Copy link
Owner

cinar commented Apr 15, 2024

Hi Thomas-Heniart, thank you for reporting the issue. It's been a while since I implemented it, so I'll need to refresh my memory. If you have any reference materials or test data that could be helpful, I'd really appreciate it.

@Thomas-Heniart
Copy link
Contributor Author

Hi @cinar, thanks for your prompt answer!
If that suits you, I was thinking about doing it myself and opening a pull request.
What do you think about it?

@cinar
Copy link
Owner

cinar commented Apr 16, 2024

Wow, that would be awesome!

@Thomas-Heniart
Copy link
Contributor Author

Thomas-Heniart commented Apr 16, 2024

Hi @cinar, I've got a new implementation respecting the original Ichimoku indicators calculation ready and covered with tests.
It still requires some refactoring and algorithm improvements.
Could you give me the right to create a branch and open a PR? That would be lovely 🙏
PS: My bad, I forgot I could just fork the repository and open a PR based on it 👍

Thomas-Heniart added a commit to Thomas-Heniart/indicatorts that referenced this issue Apr 16, 2024
Thomas-Heniart added a commit to Thomas-Heniart/indicatorts that referenced this issue Apr 17, 2024
@cinar
Copy link
Owner

cinar commented Apr 17, 2024

Thank you very much! I put a minor comment. Please let me know when it is good to review/merge.

Thomas-Heniart added a commit to Thomas-Heniart/indicatorts that referenced this issue Apr 18, 2024
cinar pushed a commit that referenced this issue Apr 18, 2024
Fix #452

- [x] Fix indicators calculation
- [x] Reduce duplication
- [x] Update documentation
Thomas-Heniart added a commit to Thomas-Heniart/indicatorts that referenced this issue Apr 23, 2024
@Thomas-Heniart
Copy link
Contributor Author

Hi @cinar
I've just spotted a bug I introduced when trying to fix the laggingSpan. I've opened a new PR that fixes it.

@cinar
Copy link
Owner

cinar commented Apr 24, 2024

Thank you very much Thomas! Let me quickly check and merge it then!

cinar pushed a commit that referenced this issue Apr 24, 2024
# Describe Request

The shift left function was incorrect

Fixed #452 

# Change Type

Fix function
@Thomas-Heniart
Copy link
Contributor Author

That was incredibly fast, thanks a lot 🙏

@Thomas-Heniart
Copy link
Contributor Author

Hi @cinar, could you update the package version so npm publishes a new one?
I didn't do it in my PR because I don't know if an external contributor is allowed to.

@cinar
Copy link
Owner

cinar commented Apr 25, 2024

Hi @Thomas-Heniart , I just pushed v2.2.1 with the fix. Just FYI.

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