-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Smoothed Moving Average (SMMA) Strategy added. #249
Conversation
WalkthroughThis pull request introduces the Smoothed Moving Average (SMMA) Strategy to the Indicator Go module. The changes include adding a new strategy type Changes
Sequence DiagramsequenceDiagram
participant Asset as Asset Snapshots
participant SMMA as SMMA Strategy
participant Compute as Compute Method
participant Action as Action Channel
Asset->>SMMA: Provide snapshots
SMMA->>Compute: Process snapshots
Compute->>Compute: Calculate short and long SMMA
Compute->>Action: Generate trading actions
Action-->>SMMA: Return Buy/Sell/Hold recommendations
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #249 +/- ##
==========================================
+ Coverage 93.54% 93.59% +0.05%
==========================================
Files 173 174 +1
Lines 6101 6180 +79
==========================================
+ Hits 5707 5784 +77
- Misses 335 337 +2
Partials 59 59 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (3)
strategy/trend/smma_strategy.go (2)
24-27
: Correct spelling of "Smoothed" in the documentation.There is a minor spelling inconsistency: "Smooted" vs. "Smoothed." Correcting this ensures clarity.
-// Smooted Moving Averge (SMMA) strategy. A short-term SMMA crossing above +// Smoothed Moving Average (SMMA) strategy. A short-term SMMA crossing above
125-130
: Column naming consistency.Using “MACD” and “Signal” columns might be a leftover from another strategy. Here they should be more descriptive of SMMA values, e.g. “Short SMMA” and “Long SMMA,” to avoid confusion.
-report.AddColumn(helper.NewNumericReportColumn("MACD", shortSmmas), 1) -report.AddColumn(helper.NewNumericReportColumn("Signal", longSmmas), 1) +report.AddColumn(helper.NewNumericReportColumn("Short SMMA", shortSmmas), 1) +report.AddColumn(helper.NewNumericReportColumn("Long SMMA", longSmmas), 1)strategy/trend/README.md (1)
793-854
: Fix typos in SMMA strategy documentationThere are spelling errors in the documentation:
- "Smooted Moving Averge" should be "Smoothed Moving Average"
- // short-term Smooted Moving Averge (SMMA). + // short-term Smoothed Moving Average (SMMA). - // long-term Smooted Moving Averge (SMMA). + // long-term Smoothed Moving Average (SMMA).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
strategy/trend/testdata/smma_strategy.csv
is excluded by!**/*.csv
📒 Files selected for processing (6)
README.md
(1 hunks)strategy/trend/README.md
(3 hunks)strategy/trend/smma_strategy.go
(1 hunks)strategy/trend/smma_strategy_test.go
(1 hunks)strategy/trend/trend.go
(1 hunks)trend/README.md
(4 hunks)
🔇 Additional comments (5)
strategy/trend/trend.go (1)
36-36
: Great addition to the list of trend strategies.
This integrates the Smoothed Moving Average (SMMA) strategy into the existing AllStrategies
function. Everything looks good here.
strategy/trend/smma_strategy_test.go (1)
39-56
: Effective reporting test coverage.
• The creation of the HTML file and subsequent cleanup with defer os.Remove(fileName)
is neat.
• Ensure test outputs are included in .gitignore so that temporary files do not clutter the repo.
README.md (1)
125-125
: Accurate insertion of SMMA strategy reference.
The new bullet for “Smoothed Moving Average (SMMA) Strategy” fits well among other trend strategies.
strategy/trend/README.md (1)
88-93
: LGTM: Index updated correctly with SMMA strategy
The SmmaStrategy type and its methods are properly documented in the index section, following the existing pattern and style.
trend/README.md (1)
Line range hint 1478-1505
: LGTM: SMMA implementation is well-documented
The Smma type documentation and implementation:
- Provides clear formula explanation
- Includes usage examples
- Follows project conventions for function signatures
- Maintains consistency with other moving average implementations
func TestSmmaStrategy(t *testing.T) { | ||
snapshots, err := helper.ReadFromCsvFile[asset.Snapshot]("testdata/brk-b.csv", true) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
results, err := helper.ReadFromCsvFile[strategy.Result]("testdata/smma_strategy.csv", true) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
|
||
expected := helper.Map(results, func(r *strategy.Result) strategy.Action { return r.Action }) | ||
|
||
smma := trend.NewSmmaStrategy() | ||
actual := smma.Compute(snapshots) | ||
|
||
err = helper.CheckEquals(actual, expected) | ||
if err != nil { | ||
t.Fatal(err) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Test data files are present but paths need to be updated
The test is using incorrect relative paths. The actual files are located at:
- "strategy/trend/testdata/brk-b.csv" instead of "testdata/brk-b.csv"
- "strategy/trend/testdata/smma_strategy.csv" instead of "testdata/smma_strategy.csv"
The test needs to be updated to use the correct paths relative to the test file location.
🔗 Analysis chain
Solid test coverage for SMMA strategy computation.
• The CSV-based testing approach provides strong validation.
• Good use of helper functions for reading input and verifying outputs.
• Consider verifying that “testdata/smma_strategy.csv” is present and correctly named in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify existence of smma_strategy.csv and brk-b.csv in testdata directory
fd smma_strategy.csv --exact-depth 2 --search-path testdata
fd brk-b.csv --exact-depth 2 --search-path testdata
Length of output: 307
Script:
#!/bin/bash
# Let's first find where the test file is located to determine the correct path
fd smma_strategy_test.go
# Then search for CSV files in the repository
fd -e csv
Length of output: 3922
Describe Request
Smoothed Moving Average (SMMA) Strategy added.
Change Type
New Strategy.
Summary by CodeRabbit
New Features
Bug Fixes
Tests