search

cinar / indicatorts

IndicatorTS - Stock technical indicators and strategies in TypeScript for browser and server programs.
MIT License
283 stars 48 forks source link

Ichimoku indicators are not completely correct #452

Closed Thomas-Heniart closed 4 months ago

Thomas-Heniart commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Wow, that would be awesome!

Thomas-Heniart commented 5 months ago

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 👍

cinar commented 5 months ago

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

Thomas-Heniart commented 4 months ago

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 commented 4 months ago

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

Thomas-Heniart commented 4 months ago

That was incredibly fast, thanks a lot 🙏

Thomas-Heniart commented 4 months ago

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 commented 4 months ago

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