Added support for SMMA. #247
Conversation
|
Warning Rate limit exceeded@cinar has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis pull request introduces the Smoothed Moving Average (SMMA) indicator to the technical analysis library. The implementation includes adding documentation in the README.md, creating a new Changes
Assessment against linked issues
Poem
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #247 +/- ##
==========================================
+ Coverage 93.51% 93.54% +0.02%
==========================================
Files 172 173 +1
Lines 6074 6101 +27
==========================================
+ Hits 5680 5707 +27
Misses 335 335
Partials 59 59 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
trend/smma.go (4)
13-16: Name the default constant for clarity
The DefaultSmmaPeriod = 7 is well-chosen. Keep in mind it may require documentation if users want the rationale for 7 as the default.
18-19: Consider docstring format
The triple-slash doc comments may not be recognized by all doc generators. Confirm the doc tool compatibility or convert them to standard Go doc comment style.
29-32: Struct design
“Period int” is sufficient for the SMMA’s current usage, but if future scaling is expected (e.g., weighting factors or advanced smoothing constants), ensure the struct remains open for extension.
41-47: Parameter validation
Consider validating the “period” argument (e.g., must be ≥ 1) in NewSmmaWithPeriod, returning an error or default period if out of range.func NewSmmaWithPeriod[T helper.Number](period int) *Smma[T] { + if period < 1 { + period = DefaultSmmaPeriod + } smma := NewSmma[T]() smma.Period = period return smma }trend/README.md (1)
1442-1512: Fix minor documentation lint warnings
Fenced code blocks lacking language spec can trigger markdownlint MD040. Adding “go” or “bash” (depending on snippet) helps.- ``` + ```go smma := trend.NewSmma[float64]() smma.Period = 10 result := smma.Compute(c)🧰 Tools
🪛 LanguageTool
[uncategorized] ~1483-~1483: Possible missing article found.
Context: ...elper.Number](period int) *Smma[T] ``` NewSmmaWithPeriod function initializes a new SMMA instanc...(AI_HYDRA_LEO_MISSING_THE)
🪛 Markdownlint (0.37.0)
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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
trend/testdata/smma.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
README.md(1 hunks)trend/README.md(3 hunks)trend/smma.go(1 hunks)trend/smma_test.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
trend/README.md
[uncategorized] ~1483-~1483: Possible missing article found.
Context: ...elper.Number](period int) *Smma[T] ``` NewSmmaWithPeriod function initializes a new SMMA instanc...
(AI_HYDRA_LEO_MISSING_THE)
🪛 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 (7)
trend/smma_test.go (4)
25-27: Validate duplication and mapping
Using helper.Duplicate and helper.Map is a neat pattern. Just ensure these function calls preserve critical data (e.g., no accidental mismatch of indices).
34-39: Idle period usage
Skipping values by smma.IdlePeriod() works well. Just ensure you have coverage in your tests to confirm that the offset is applied consistently.
14-19: Ensure CSV file presence and correct input reading
This new test depends on "testdata/smma.csv". Please verify that the CSV file stands at the specified path within the repository.
✅ Verification successful
CSV file is present at the expected location
The file "smma.csv" exists at trend/testdata/smma.csv with appropriate read permissions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if smma.csv file is present in the testdata directory.
fd smma.csv --exec ls -l
Length of output: 101
29-33: Compute & rounding approach
Using helper.RoundDigits(actual, 2) aligns the computed results with typical financial decimals. However, consider verifying edge cases (e.g., negative values or extremely large values) in separate tests.
trend/smma.go (1)
50-69: Potential concurrency pitfalls
SMMA spawns a goroutine, which is standard. However, ensure the calling context properly consumes the returned channel. If any consumer reads partially and stops, the goroutine may block. Consider including context cancellation or ensuring you read all values from the channel.
✅ Verification successful
Let me analyze the codebase to understand the context better and verify the concurrency concerns.
This will help us understand:
- If context cancellation is already implemented somewhere
- How the channels are consumed in tests
- Locate test files to analyze usage patterns
Let's examine the test file to understand how the channel is consumed and if there are any existing patterns for handling channel cleanup.
This will help us:
- Understand how the SMMA channel is consumed in tests
- Check if there are any helper functions for safe channel consumption
Let's check the helper package for channel-related functions since the path was incorrect in the previous attempt.
This will help us understand:
- The correct location of helper functions
- How channels are handled in helper functions
- The implementation of Head function which is used in SMMA's Compute method
Let me check the Head function implementation specifically since it's used in the SMMA's Compute method.
This will help us understand:
- How the Head function handles channel operations
- How the test cases handle channel consumption and cleanup
Based on the analysis of the code, I can now generate the final response:
Channel handling is properly implemented
The SMMA implementation follows the library's established patterns for safe channel handling. The Head function properly closes its result channel using defer close(result), and the SMMA's Compute method also ensures proper channel closure. The test demonstrates that the channels are fully consumed through helper functions like CheckEquals, which prevents goroutine leaks. The concern about partial reads is mitigated by the library's design pattern where channels are always fully consumed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any context.Context usage in trend/smma.go and related files
rg -l "context\.Context" --type go
# Look for channel consumption patterns in test files
rg -l "Smma.*Compute.*chan" --type go
# Find test files related to SMMA
fd "smma.*test.*\.go$"
Length of output: 134
Script:
#!/bin/bash
# Check the content of the test file
cat trend/smma_test.go
# Look for any helper functions that might be used for channel consumption
rg -A 5 "func.*chan" trend/helper/
Length of output: 1001
Script:
#!/bin/bash
# Look for helper package location
fd "helper"
# Check for channel-related functions in helper package
rg -A 5 "func.*chan" helper/
# Look specifically for Head function implementation since it's used in SMMA
ast-grep --pattern 'func Head($$$) $$$'
Length of output: 41994
Script:
#!/bin/bash
# Check the implementation of Head function
cat helper/head.go
# Look for any error handling or cleanup patterns in the test file
cat trend/smma_test.go
Length of output: 1609
README.md (1)
51-51: SMMA listing
Adding the Smoothed Moving Average (SMMA) to the Trend Indicators section is coherent.
trend/README.md (1)
309-316: License doc placement
The default SMMA period is documented here. This is correct. Confirm that the license block at lines 309–316 aligns with your project’s standard license disclaimers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
trend/smma.go (3)
39-44: Validate the 'period' parameter.Currently, there's no validation for the period parameter. If a user provides a non-positive period (e.g., 0 or negative), the SMMA computation may panic or yield undefined behavior.
You can add a sanity check to ensure that the period is greater than zero:
func NewSmmaWithPeriod[T helper.Number](period int) *Smma[T] { + if period <= 0 { + panic("SMMA period must be greater than 0") + } return &Smma[T]{ Period: period, } }
47-66: Consider buffering and concurrency constraints.You're calling “cap(c)” when creating the result channel. If “c” has no capacity and is blocking, the resulting channel might also block or cause suboptimal performance. Also, ensure that the concurrency flow suits your use case—especially if “c” has a large capacity or is unbuffered.
68-71: Refine the documentation for IdlePeriod.The comment “IdlePeriod is the initial period that SMMA yield any results” can be clarified for better readability.
For example:
“It returns the number of initial data points required before SMMA yields any valid result.”trend/smma_test.go (2)
20-23: Handle potential missing test data.If "testdata/smma.csv" is missing or corrupted, this test fails immediately. You might want to handle the scenario gracefully or add a more descriptive error message for debugging.
34-37: Add targeted coverage tests for edge and negative cases.Currently, the test checks typical SMMA calculations, but there’s no test coverage for edge or erroneous conditions (e.g., zero/negative period or empty input). Expanding the test suite would help maintain coverage quality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
trend/smma.go(1 hunks)trend/smma_test.go(1 hunks)
🔇 Additional comments (1)
trend/smma.go (1)
60-60:
Correct the SMMA formula.
The intended formula for SMMA is “(previous * (N - 1) + newClose) / N”. The current code subtracts 1 from “before*T(s.Period)” instead of “T(s.Period) - 1”.
Apply this diff to correct the formula:
-before = ((before*T(s.Period) - 1) + n) / T(s.Period)
+before = ((before * (T(s.Period) - 1)) + n) / T(s.Period)Note: This aligns with the well-known SMMA definition.
Likely invalid or redundant comment.
Describe Request
Added support for Smoothed Moving Average (SMMA).
Fixed #246
Change Type
New Indicator.
Summary by CodeRabbit
New Features
Tests