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

Bollinger Bands can now accept different smoothing (Moving Averages) #662

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

o-nazaruk
Copy link

@o-nazaruk o-nazaruk commented Apr 2, 2024

Checklist for new indicators

  • Indicator is documented and implemented by extending BigIndicatorSeries
  • A "faster" version of this indicator is implemented by extending NumberIndicatorSeries
  • Tests for getResult are present
  • Tests for highest and lowest result caching are present (if not already existent for base class)
  • Tests for error handling are present (if not already existent for base class)
  • Indicators (standard & faster version) are exposed in src/index.ts
  • Indicators (standard & faster version) are added to startBenchmark.ts
  • Indicator is listed in README.md

@o-nazaruk o-nazaruk closed this Apr 2, 2024
@o-nazaruk o-nazaruk reopened this Apr 2, 2024
@codecov-commenter
Copy link

Codecov Report

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

Project coverage is 97.87%. Comparing base (ebb6c01) to head (7c995a5).
Report is 38 commits behind head on main.

Files Patch % Lines
src/WSMA/WSMA.ts 14.28% 24 Missing ⚠️
src/EMA/EMA.ts 14.81% 23 Missing ⚠️
src/SMA/SMA.ts 57.14% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #662      +/-   ##
===========================================
- Coverage   100.00%   97.87%   -2.13%     
===========================================
  Files           36       36              
  Lines         2290     2357      +67     
  Branches       248      249       +1     
===========================================
+ Hits          2290     2307      +17     
- Misses           0       50      +50     
Flag Coverage Δ
unittests 97.87% <31.50%> (-2.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

package.json Outdated Show resolved Hide resolved
@bennycode
Copy link
Owner

Hi @o-nazaruk, do you need any help resolving the code conflicts or writing tests for it?

@bennycode
Copy link
Owner

Hey @o-nazaruk, are you still interested in getting this merged? I think it is a very good change that you proposed here. :)

@o-nazaruk
Copy link
Author

Hello @bennycode !

I haven't had a chance to have the tests developed for now. It's been quite busy for me haha.
Up to you in terms of if you want the tests done sooner. I don't know when I will have free time - but once I am free I will certainly improve coverage

@bennycode
Copy link
Owner

@o-nazaruk can you create a new PR that add the smoothing parameter to the constructur of Bollinger Bands? I can take over from there.

@bennycode
Copy link
Owner

Hi @o-nazaruk, I am trying to fix your PR to match the latest "main" branch. One thing that I noticed is that you turned getResultFromBatch into an instance method instead of keeping it static. Let's try to run with the static method as it is not necessary to create an instance of an indicator with a specific interval first. We just have to figure out how we can make a static method part of the MovingAverage abstract class.

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.

None yet

3 participants