Conversation
WalkthroughAdds a new generic ROC indicator type with default period, constructors, Compute stream using a ring buffer and helper utilities, IdlePeriod/String methods, and unit tests (vector and CSV-driven) validating outputs after skipping the idle period and rounding. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Roc
participant Ring as helper.NewRing
participant Map as helper.Map
participant Skip as helper.Skip
participant Out as Output Channel
Caller->>Roc: NewRoc / NewRocWithPeriod
Caller->>Roc: Compute(values <-chan)
Roc->>Ring: create ring buffer (Period)
Roc->>Map: for each value: when ring full -> compute (value - prev) / prev
Roc->>Ring: push current value
Roc->>Skip: skip first IdlePeriod outputs
Skip-->>Out: emit ROC stream
Out-->>Caller: consume ROC values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (1)
trend/roc.go (1)
66-69: Fix the string representation comment.The comment incorrectly references "WMA" instead of "ROC".
-// String is the string representation of the WMA. +// String is the string representation of the ROC.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
trend/testdata/roc.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
trend/roc.go(1 hunks)trend/roc_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
trend/roc_test.go (3)
trend/roc.go (2)
NewRoc(27-29)Roc(21-24)helper/skip.go (1)
Skip(15-30)helper/csv.go (1)
ReadFromCsvFile(278-285)
trend/roc.go (1)
helper/skip.go (1)
Skip(15-30)
🔇 Additional comments (7)
trend/roc_test.go (2)
9-23: LGTM! Simple test correctly validates ROC calculation.The test correctly sets up a simple scenario with a closing price stream and validates the ROC calculation with a period of 3. The expected values are correctly calculated and the test properly handles the idle period alignment.
25-51: LGTM! Data-driven test provides comprehensive coverage.The test effectively validates the ROC indicator using CSV test data, properly handles channel duplication, and includes rounding to handle floating-point precision. The test structure follows established patterns in the codebase.
trend/roc.go (5)
13-16: LGTM! Default period aligns with common ROC usage.The default period of 9 is a reasonable choice for the ROC indicator and is commonly used in technical analysis.
26-29: LGTM! Constructor follows established patterns.The constructor correctly delegates to the parameterized version with the default period.
31-36: LGTM! Parameterized constructor is well-structured.The constructor properly initializes the ROC struct with the given period parameter.
38-59: LGTM! ROC computation is correctly implemented.The implementation correctly:
- Uses a ring buffer to maintain the sliding window of values
- Calculates ROC as (current - previous) / previous when sufficient data is available
- Skips the idle period to align output properly
- Handles the case when the window is not yet full
The logic matches the ROC indicator specification from the linked issue.
61-64: LGTM! Idle period correctly returns the configured period.The implementation correctly returns the period as the idle period, which aligns with the ROC calculation needing that many periods of historical data.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #277 +/- ##
=======================================
Coverage 91.66% 91.67%
=======================================
Files 181 182 +1
Lines 6540 6570 +30
=======================================
+ Hits 5995 6023 +28
- Misses 484 486 +2
Partials 61 61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
trend/roc_test.go (2)
9-23: Add assertion for String() to cover and validate representation.You can cheaply bump patch coverage and validate the public API by asserting the string format after setting Period.
Apply this diff:
roc := NewRoc[float64]() roc.Period = 3 + if s := roc.String(); s != "ROC(3)" { + t.Fatalf("unexpected String(): %s", s) + }
40-41: Assert IdlePeriod matches the default to improve coverage and guard API.This will exercise IdlePeriod() and the default constant, addressing part of the Codecov gap.
Apply this diff:
roc := NewRoc[float64]() + if roc.IdlePeriod() != DefaultRocPeriod { + t.Fatalf("unexpected IdlePeriod: %d", roc.IdlePeriod()) + }trend/roc.go (3)
18-21: Clarify ROC documentation and fractional vs. percentage form.Issue #57 describes ROC as a percentage change. The implementation returns a fractional change (matching tests). Make this explicit to avoid confusion.
Apply this diff:
// Roc represents the configuration parameters for calculating the Rate Of Change (ROC) indicator. // -// ROC = (Current Price - Price n periods ago) / Price n periods ago +// ROC = (Current Price - Price n periods ago) / Price n periods ago +// +// Note: This implementation returns a fractional change (e.g., 0.05 == 5%). +// Multiply by 100 if you need the percentage form.If you prefer percentage output by default (per the issue wording), we can update Compute to multiply by 100 and adjust tests/CSV accordingly. Confirm the intended convention for this repo.
38-39: Fix comment: no “signal line” is computed.The method returns only ROC values.
Apply this diff:
-// Compute function takes a channel of numbers and computes the ROC and the signal line. +// Compute calculates the Rate of Change (ROC) over the configured period.
66-69: Correct docstring to reference ROC (not WMA).Copy/paste typo.
Apply this diff:
-// String is the string representation of the WMA. +// String is the string representation of the ROC.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
trend/testdata/roc.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
trend/roc.go(1 hunks)trend/roc_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
trend/roc.go (2)
helper/skip.go (1)
Skip(15-30)trend/aroon.go (2)
NewAroon(37-41)Compute(45-67)
trend/roc_test.go (3)
trend/roc.go (2)
NewRoc(27-29)Roc(21-24)helper/skip.go (1)
Skip(15-30)helper/csv.go (1)
ReadFromCsvFile(278-285)
| func TestRocTestdata(t *testing.T) { | ||
| type Data struct { | ||
| Close float64 | ||
| Roc float64 | ||
| } | ||
|
|
||
| input, err := helper.ReadFromCsvFile[Data]("testdata/roc.csv", true) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| inputs := helper.Duplicate(input, 2) | ||
| closing := helper.Map(inputs[0], func(d *Data) float64 { return d.Close }) | ||
| expected := helper.Map(inputs[1], func(d *Data) float64 { return d.Roc }) | ||
|
|
||
| roc := NewRoc[float64]() | ||
|
|
||
| actual := roc.Compute(closing) | ||
| actual = helper.RoundDigits(actual, 2) | ||
|
|
||
| expected = helper.Skip(expected, roc.IdlePeriod()) | ||
|
|
||
| err = helper.CheckEquals(actual, expected) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding a targeted test for zero-division and NewRocWithPeriod constructor.
- Division by zero is currently not guarded in roc.go; if p == 0, this test will catch the edge-case once the guard is added.
- Explicitly tests NewRocWithPeriod to cover constructor and String().
Add this new test function:
func TestRocWithZeroDivisionAndConstructor(t *testing.T) {
// previous value (n periods ago) is zero -> ROC should be 0 (no panic/inf)
closing := helper.SliceToChan([]float64{0, 1, 2, 3})
roc := NewRocWithPeriod[float64](3)
if s := roc.String(); s != "ROC(3)" {
t.Fatalf("unexpected String(): %s", s)
}
if roc.IdlePeriod() != 3 {
t.Fatalf("unexpected IdlePeriod: %d", roc.IdlePeriod())
}
actual := roc.Compute(closing)
expected := helper.SliceToChan([]float64{0})
err := helper.CheckEquals(actual, expected)
if err != nil {
t.Fatal(err)
}
}This should close the remaining coverage gap Codecov reported (String() and NewRocWithPeriod()) and guard a critical numeric edge case.
🤖 Prompt for AI Agents
In trend/roc_test.go around lines 25 to 51, add a new unit test that exercises
NewRocWithPeriod and the zero-division edge-case: create a closing series where
the value from n periods ago is zero, instantiate roc with
NewRocWithPeriod[float64](3), assert roc.String() == "ROC(3)" and
roc.IdlePeriod() == 3, compute ROC over the input (use helper.SliceToChan) and
assert the result yields 0 (no panic/Inf) using helper.CheckEquals;
additionally, update roc.go to guard the division so when the previous value is
zero the function returns 0 for that ROC element instead of dividing by zero.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
trend/roc.go (4)
18-21: Clarify “percentage vs. fraction” in the ROC formula comment.Implementation returns a fractional ROC (no ×100). The PR/issue text says “percentage,” which could confuse users.
Consider updating the doc to explicitly state the fractional form and how to get percent:
-// ROC = (Current Price - Price n periods ago) / Price n periods ago +// ROC = (Current Price - Price n periods ago) / Price n periods ago +// +// Note: Returns fractional ROC (e.g., 0.05 == 5%). Multiply by 100 if percentage is desired.Please confirm this matches conventions used in other indicators in this repo. I can align related docs/tests if needed.
41-42: Fix copy/paste in Compute’s doc (“signal line” is not present).Apply:
-// Compute function takes a channel of numbers and computes the ROC and the signal line. +// Compute computes the ROC over the input stream.
48-53: Drop redundant ok check after IsFull().If the ring is full, Get should consistently succeed; the extra ok check adds noise.
- if window.IsFull() { - p, ok := window.Get() - if ok && p != 0 { - result = (value - p) / p - } - } + if window.IsFull() { + p, _ := window.Get() + if p != 0 { + result = (value - p) / p + } + }If Get can still fail in a “full” ring per helper semantics, ignore this suggestion.
48-53: Add a test for the p == 0 branch to improve patch coverage and guard behavior.Codecov reports a few uncovered lines here. A targeted test with period=1 and an initial zero will exercise the division-by-zero guard:
// In trend/roc_test.go func TestRocDivisionByZeroGuard(t *testing.T) { roc := NewRocWithPeriod[float64](1) values := []float64{0, 1, 2} in := make(chan float64, len(values)) for _, v := range values { in <- v } close(in) out := roc.Compute(in) var got []float64 for v := range out { got = append(got, v) } // period=1 -> skip first element; expect two outputs // First computed step divides by previous=0 -> guarded to 0.0 // Second step: (2-1)/1 = 1.0 want := []float64{0.0, 1.0} if len(got) != len(want) { t.Fatalf("length mismatch: got %d, want %d", len(got), len(want)) } for i := range want { if math.Abs(got[i]-want[i]) > 1e-9 { t.Fatalf("got[%d]=%v, want=%v", i, got[i], want[i]) } } }I can add a companion test to assert warm-up skipping for a small period if you’d like.
Also applies to: 59-61
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
trend/roc.go(1 hunks)trend/roc_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- trend/roc_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
trend/roc.go (1)
helper/skip.go (1)
Skip(15-30)
🔇 Additional comments (4)
trend/roc.go (4)
31-39: Good input validation for period.Clamping period <= 0 to DefaultRocPeriod avoids undefined ring behavior. Matches prior review feedback.
45-57: Core ROC computation and warm-up handling look correct.
- Using a ring to access the value n periods ago is appropriate.
- Off-by-one is handled correctly with Skip(r.Period), so the first emitted value corresponds to the first valid ROC.
- Guarding division by zero avoids Inf/NaN leakage during zero prices.
59-61: Idle period skipping is correct.Skip(r.Period) cleanly removes the initial warm-up window from the output stream.
69-72: String() format is consistent and useful."ROC(%d)" aligns with other indicators and is test-friendly.
|
Thank you very much! |
3 participants
Describe Request
Add momentum indicator ROC
Fixed #57
Change Type
What is the type of this change.
Summary by CodeRabbit
New Features
Tests