Smoothed Moving Average (SMMA) Strategy added. #248
Conversation
WalkthroughThis pull request introduces the Smoothed Moving Average (SMMA) as a new technical indicator and strategy to the trend analysis module. The implementation spans multiple files across the Changes
Sequence DiagramsequenceDiagram
participant Asset as Asset Snapshots
participant SMMA as SMMA Indicator
participant Strategy as SMMA Strategy
participant Actions as Trading Actions
Asset ->> SMMA: Provide price data
SMMA -->> SMMA: Calculate Smoothed Moving Average
SMMA ->> Strategy: Pass SMMA values
Strategy ->> Strategy: Compare short and long periods
Strategy ->> Actions: Generate Buy/Sell/Hold recommendation
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
trend/smma_test.go (2)
14-19: Check for potential CSV reading failure scenarios.The test is straightforward, but consider negative or edge test cases with missing values, no data, or malformed CSV rows. This ensures robust handling of real-world data.
29-40: Ensure correct idle-period offset validations.You've correctly skipped values corresponding to the SMMA’s idle period before comparing with expected results. Consider adding a small test to confirm the skip logic if the idle period is changed.
strategy/trend/smma_strategy_test.go (1)
38-55: Comprehensive reporting test.Generating an HTML report is a nice approach. If feasible, include an automated check or golden file comparison for the generated report to catch formatting regressions.
strategy/trend/smma_strategy.go (3)
25-27: Fix typos in “Smooted Moving Averge” references.
There are spelling errors in the comments: “Smooted” → “Smoothed” and “Averge” → “Average.”- // Smooted Moving Averge (SMMA). + // Smoothed Moving Average (SMMA).Also applies to: 29-31, 34-35
74-74: Fix minor spelling error in the comment.
“short-perios” should be corrected to “short-period.”- // A short-perios SMMA value crossing above ... + // A short-period SMMA value crossing above ...
128-130: Rename misleading column labels.
"MACD" and "Signal" columns should match SMMA terminology 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)trend/README.md (1)
1452-1459: Add language spec to fenced code blocks.
Markdownlint flagged code blocks lacking a language identifier. Specify a language to comply with style guidelines (e.g., ```go).Example fix:
- ``` + ```goAlso applies to: 1474-1475
🧰 Tools
🪛 Markdownlint (0.37.0)
1454-1454: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
strategy/trend/testdata/smma_strategy.csvis excluded by!**/*.csvtrend/testdata/smma.csvis excluded by!**/*.csv
📒 Files selected for processing (9)
README.md(2 hunks)strategy/testdata/x(0 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(3 hunks)trend/smma.go(1 hunks)trend/smma_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- strategy/testdata/x
🧰 Additional context used
🪛 Markdownlint (0.37.0)
trend/README.md
1447-1447: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
1454-1454: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (12)
strategy/trend/trend.go (1)
36-36: Add an accompanying test for the new SMMA strategy inclusion.
This newly added call to NewSmmaStrategy is good; however, please ensure test coverage by verifying this new strategy is invoked in integration or e2e tests (if available) to confirm it behaves as expected among existing strategies.
trend/smma_test.go (3)
1-4: Retain a clear license header and project metadata.
These lines properly declare the licensing terms and reference the project repository. No issues detected.
20-28: Verify file path correctness & potential test data issues.
Reading from “testdata/smma.csv” is fine. Ensure the CSV truly has enough columns for both close and SMMA columns, especially across multiple environments.
42-49: Maintain naming consistency in SMMA string representation tests.
The test method name and logic are clear. The same approach for verifying string representation could be extended to other indicators for consistency.
strategy/trend/smma_strategy_test.go (2)
1-4: License & package declaration look good.
No concerns with license format or package naming.
16-36: Validate data completeness in CSV files for strategy checks.
The approach to reading snapshots and result data from CSV is adequate. Validate that the CSV files contain consistent and comprehensive data sets for both snapshots and expected results, especially for varied market scenarios (e.g., bullish, bearish, or flat).
trend/smma.go (3)
1-4: License header alignment.
Maintains the project’s license and documentation approach. Looks consistent with other files.
13-16: Confirm default period alignment with domain expectations.
The default SMMA period of 7 is acceptable. Ensure it aligns with common usage or domain references, as some systems might prefer different defaults (e.g., 9, 10, or 14).
68-76: Idle period & string representation look correct.
The computed idle period (Period - 1) and the SMMA’s string format appear correct.
README.md (1)
51-51: Documentation additions look good.
The new SMMA indicator and strategy references appear consistent and properly hyperlinked.
Also applies to: 125-125
strategy/trend/README.md (1)
88-93: SMMA strategy documentation is well-structured.
No issues found with the added lines—naming, references, and cross-links appear consistent.
trend/README.md (1)
309-316: DefaultSmmaPeriod usage is correct.
The default SMMA period of 7 aligns with the codebase patterns, no improvements needed here.
| // Compute function takes a channel of numbers and computes the SMMA over the specified period. | ||
| func (s *Smma[T]) Compute(c <-chan T) <-chan T { | ||
| result := make(chan T, cap(c)) | ||
|
|
||
| go func() { | ||
| defer close(result) | ||
|
|
||
| // Initial SMMA value is the SMA. | ||
| sma := NewSmaWithPeriod[T](s.Period) | ||
|
|
||
| before := <-sma.Compute(helper.Head(c, s.Period)) | ||
| result <- before | ||
|
|
||
| for n := range c { | ||
| before = ((before * (T(s.Period) - 1)) + n) / T(s.Period) | ||
| result <- before | ||
| } | ||
| }() | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider boundary checks on period and concurrency aspects.
- If period is less than 1, the formula might break. Add a small check or note to prevent negative or zero period values.
- The channel-based approach is sound, but document concurrency usage or let users know the channel is consumed.
| // DefaultSmmaStrategyLongPeriod is the default short-term SMMA period of 50. | ||
| DefaultSmmaStrategyLongPeriod = 50 |
There was a problem hiding this comment.
Correct the mismatch in documentation for DefaultSmmaStrategyLongPeriod.
The comment incorrectly states that 50 is the default “short-term” period, but this constant is intended for the “long-term” period.
- // DefaultSmmaStrategyLongPeriod is the default short-term SMMA period of 50.
+ // DefaultSmmaStrategyLongPeriod is the default long-term SMMA period of 50.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // DefaultSmmaStrategyLongPeriod is the default short-term SMMA period of 50. | |
| DefaultSmmaStrategyLongPeriod = 50 | |
| // DefaultSmmaStrategyLongPeriod is the default long-term SMMA period of 50. | |
| DefaultSmmaStrategyLongPeriod = 50 |
Describe Request
Smoothed Moving Average (SMMA) Strategy added.
Change Type
New Strategy.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation