cinar / indicatorts

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

Fix ichimoku indicators calculation (#452) #455

Closed Thomas-Heniart closed 4 months ago

Thomas-Heniart commented 5 months ago

Fix https://github.com/cinar/indicatorts/issues/452

codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.37%. Comparing base (496c3e4) to head (5f811e4). Report is 9 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #455 +/- ## ========================================== + Coverage 65.58% 72.37% +6.79% ========================================== Files 94 102 +8 Lines 1537 1665 +128 Branches 181 254 +73 ========================================== + Hits 1008 1205 +197 + Misses 517 448 -69 Partials 12 12 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

cinar commented 4 months ago

That's a good point. Let's go with your suggestion.

On Wed, Apr 17, 2024 at 12:46 PM Thomas Heniart @.***> wrote:

@.**** commented on this pull request.

In src/indicator/momentum/ichimokuCloud.ts https://github.com/cinar/indicatorts/pull/455#discussion_r1569423209:

  • return {
  • conversion,
  • base,
  • leadingSpanA,
  • leadingSpanB,
  • laggingSpan,
  • };
  • return {
  • conversion: tenkanSen,
  • base: kijunSen,
  • leadingSpanA: calculateSenkouSpanA({tenkanSen, kijunSen, medium}),

Tenkan, Kijun, SSA, SSB, and LagginSpan terms are widely used among people working with Ichimoku. I didn't want to introduce a "breaking change" in the code base, that's why I didn't touch IchimokuCloudResult If you think it's to changeIchimokuCloudResult interface then it could be

export interface IchimokuCloudResult { kijun: number[]; tenkan: number[]; ssa: number[]; ssb: number[]; laggingSpan: number[];}

— Reply to this email directly, view it on GitHub https://github.com/cinar/indicatorts/pull/455#discussion_r1569423209, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANMH3CDUT3BYRTZZF6RYTDY53GS3AVCNFSM6AAAAABGKH25TCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMBWHE4TQOBWGQ . You are receiving this because you commented.Message ID: @.***>

Thomas-Heniart commented 4 months ago

Done @cinar, I think it's ready for review 👍

cinar commented 4 months ago

Thank you very much! I am merging it!