Adding configuration for all indicators

cereallarceny commented 5 months ago

Included in this PR:

All indicators should use an object configuration per the feature request described in this issue: Closes https://github.com/cinar/indicatorts/issues/445
Introduced shorter names for most functions (i.e. awesomeOscillator is now exported as ao and awesomeOscillator. All internal usage uses the abbreviated name if one exists. Abbreviations we're borrowed from other similar libraries.
Code coverage for indicators was brought up significantly. Strategy tests were deemed out of scope for this PR.
Documentation was altered accordingly

Considerations:

I did not place deprecation messages in this PR because, in all honesty, it's probably better reviewed as a major version bump. The amount of messages we'd have to introduce into the codebase would be significant.
One other consideration is that I added a number of default values, primarily for periods. I don't know what a lot of default values should be - so I'll be seeking guidance on what you want those values to be.
I can't vouch for the accuracy of the numbers in the tests. However, since this PR doesn't alter the integrity of the functions themselves, the only way they could be wrong is if the original logic was wrong to begin with. With that said, it wouldn't be a bad time to verify these values against other reputable libraries.
I noticed that sometimes test data was the same, even when passing a different period. This may be due to low quality mock data, but I'm not sure why this is.

cereallarceny commented 5 months ago

Just getting started on this @cinar, only finished 3, but the premise is there. Would love to see if you approve of the overall idea before I do the rest.

codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 80.00000% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 66.73%. Comparing base (496c3e4) to head (db262b4).

Files	Patch %	Lines
src/indicator/momentum/williamsR.ts	0.00%	2 Missing :warning:
src/indicator/trend/aroon.ts	0.00%	2 Missing :warning:
src/indicator/volatility/chandelierExit.ts	0.00%	2 Missing :warning:
src/indicator/volatility/projectionOscillator.ts	0.00%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #446 +/- ## ========================================== + Coverage 65.58% 66.73% +1.14% ========================================== Files 94 94 Lines 1537 1545 +8 Branches 181 186 +5 ========================================== + Hits 1008 1031 +23 + Misses 517 502 -15 Partials 12 12 ```

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

cinar commented 5 months ago

Thank you very much! This looks great! How do you think we should do the nested ones? Such as one indicator using another one internally, like SMA/EMA for example. I guess nested configs in that case?

cereallarceny commented 5 months ago

Great question @cinar . I think nested configs would work, yes. Let me see if I can whip up something there real quick and get back to you.

cereallarceny commented 5 months ago

Alright @cinar. I've got an example of this done with mmax and mmin. They're then utilized in the ichimokuCloud function. In this case, I didn't need to provide a "nested config", but it just made more sense have those variables all be at the same level.

Do this:

export interface IchimokuCloudConfig {
  short?: number;
  medium?: number;
  long?: number;
  close?: number;
}

Don't do this:

export interface IchimokuCloudConfig {
  short?: { period: number };
  medium?: { period: number };
  long?: { period: number };
  close?: { period: number };
}

Thoughts on this approach?

Furthermore, if you think this effort is worth pursuing, I'd love to do this for the rest of the codebase. But I don't wanna spend a ton of time doing this for the entire codebase if you don't think there's a reasonably high likelihood it gets merged. I've done my fair share of contributions to open-source, and know it's best to check in with the original author about their intentions for the library, before diving into a 2,000+ line change PR.

What are your thoughts on this generally? I've added an issue to hold this conversation, but am also happy to have that here on the PR.

cinar commented 5 months ago

I agree, this is more comprehensive and much more tuned to the particular indicator, while hiding its under-the-hood implementation. I went back and forth on this one. In some cases, typically with moving averages in those indicators, I've seen users wanting to swap them out from time to time. For example, using SMA in MACD instead of EMA in MACD. But certainly adding them to configurations would overcomplicate things. I like your suggestion of moving indicator-specific configurations, rather than tuning the internal pieces individually.

As you are adding them, how do you think about validating those configurations? For example, if the short period is larger than the long period in this case, should we perhaps add a method to validate them, and let the indicator give an error? In some indicators, this will likely be a no-op, but for this one, it's probably going to be very useful.

Thinking more, I was also planning to implement optimizers as part of the library later on. They were going to test different configurations for the indicators and find the best one for a given asset. Your configuration change will make that super easy too!

These PRs will definitely be merged! If you are willing to do it for the rest, that would be a huge help for the project!

cinar commented 5 months ago

As you see constant numbers in those, it would be awesome if we can make them proper constants also. I'll check for those as well.

cereallarceny commented 5 months ago

You got it! I'll get started, but this may take a bit. I like the idea of validation of parameters too, but maybe we save that for a second PR?

cereallarceny commented 5 months ago

Okay @cinar , I'm done for now. I still need to go back through and update the rest of the documentation, but I wrote a nice overview of what I did here. Please note I bumped this to version 2.0 given the amount of breaking changes. But I think you'll agree, they're all for the better. Passing tests and CI, and basically doubled or tripled the number of tests - indicators should be 100% covered at this point, but I think the sample data is weak and causing them to conflate results when they shouldn't.

Anyhow, this is good for a first review. If you like what you see, I'll wrap up the docs and we can get this show on the road!

cinar commented 5 months ago

You've certainly done a lot here, thank you very much! Thank you for also adding the missing test cases and helping to increase the code coverage. Also shaping up the exports, and definitely configurations! I definitely agree with you that it makes perfect sense to bump this to v2 now given the magnitude of the changes.

cinar commented 5 months ago

A new version of the package indicatorts (2.0.0) was published at 2024-04-12T02:48:51.131Z.

cereallarceny commented 5 months ago

Terrific @cinar ! Although I do still need to update all the documentation. My apologies, I hadn't intended that to be merged yet, just to receive an initial code review. With that said, I'll work on updating the documentation now and we can release that as 2.0.1.

cereallarceny commented 5 months ago

@cinar I have finished the remaining updates here for the documentation: https://github.com/cinar/indicatorts/pull/451

cinar / indicatorts

Adding configuration for all indicators #446

Codecov Report