Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces a new Coppock Curve momentum indicator to the momentum package. Adds a generic CoppockCurve[T] type with configurable ROC and WMA periods, implements the computation formula WMA(ROC(14) + ROC(11), 10), and includes constructors, validation, and comprehensive test coverage with testdata validation. Changes
Sequence DiagramsequenceDiagram
participant Caller
participant CoppockCurve as CoppockCurve.Compute
participant ROC1 as ROC(Period1)
participant ROC2 as ROC(Period2)
participant Sum as Summation & Scaling
participant WMA as WMA(Period)
participant Output as Output Channel
Caller->>CoppockCurve: values channel
CoppockCurve->>ROC1: Start ROC(14) stream
CoppockCurve->>ROC2: Start ROC(11) stream
ROC1-->>CoppockCurve: ROC1 values
ROC2-->>CoppockCurve: ROC2 values
CoppockCurve->>Sum: Align ROC streams
Sum->>Sum: Scale by 100 & sum
Sum->>WMA: Summed values
WMA-->>CoppockCurve: WMA(10) results
CoppockCurve-->>Output: Final stream
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #331 +/- ##
==========================================
+ Coverage 93.14% 93.19% +0.04%
==========================================
Files 208 209 +1
Lines 5849 5889 +40
==========================================
+ Hits 5448 5488 +40
Misses 321 321
Partials 80 80 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@momentum/coppock_curve.go`:
- Around line 25-35: Update the doc comment above the CoppockCurve type to use
explicit fenced-code languages: wrap the formula line with a plain text fence
and the example with a ```go fenced block (i.e., replace the unlabeled
triple-backtick blocks with "```text" for the formula and "```go" for the
example), ensure the example code lines begin with // inside the comment and
close the fences, then regenerate the README/docs so the labeled fences appear
in momentum/README.md; locate the comment immediately preceding the
CoppockCurve[T helper.Float] struct to make the edits.
- Around line 74-113: Compute and IdlePeriod currently read the raw public
fields (RocPeriod1, RocPeriod2, WmaPeriod) which can be zero/invalid if the
struct is zero-valued or mutated; normalize these periods at the start of both
Compute and IdlePeriod (e.g., derive local maxRocPeriod, roc1Period, roc2Period,
wmaPeriod from the fields but applying the same defaulting/validation logic used
in NewCoppockCurveWithPeriods) so all downstream calls (helper.Buffered,
trend.NewRocWithPeriod, trend.NewWmaWith, helper.Skip and the IdlePeriod
formula) use validated positive periods; reference the symbols RocPeriod1,
RocPeriod2, WmaPeriod, Compute, IdlePeriod, and NewCoppockCurveWithPeriods when
implementing the single normalization path.
- Line 35: The generic constraint on CoppockCurve is too narrow — change the
type parameter from helper.Float to helper.Number so the indicator accepts both
Integer and Float inputs; update the struct declaration for CoppockCurve[T
helper.Number] and similarly replace helper.Float with helper.Number in the
other affected indicators (Rvi, Fisher, ConnorsRsi, PringsSpecialK) to keep the
package consistent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
momentum/testdata/coppock_curve.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
README.mdmomentum/README.mdmomentum/coppock_curve.gomomentum/coppock_curve_test.go
Comment on lines
+25
to
+35
| // CoppockCurve represents the configuration parameters for calculating the | ||
| // Coppock Curve oscillator. The Coppock Curve is a momentum indicator | ||
| // used to identify long-term buying opportunities in equity indices. | ||
| // | ||
| // Coppock Curve = WMA(ROC(14) + ROC(11), 10) | ||
| // | ||
| // Example: | ||
| // | ||
| // cc := momentum.NewCoppockCurve[float64]() | ||
| // result := cc.Compute(closings) | ||
| type CoppockCurve[T helper.Float] struct { |
Contributor
There was a problem hiding this comment.
Use explicit code-fence languages in the doc comment.
The current comment formatting generates unlabeled fenced blocks in momentum/README.md (markdownlint MD040). Add explicit text/go fences in the source comment, then regenerate docs.
📝 Suggested doc-comment adjustment
// CoppockCurve represents the configuration parameters for calculating the
// Coppock Curve oscillator. The Coppock Curve is a momentum indicator
// used to identify long-term buying opportunities in equity indices.
//
-// Coppock Curve = WMA(ROC(14) + ROC(11), 10)
+// ```text
+// Coppock Curve = WMA(ROC(14) + ROC(11), 10)
+// ```
//
// Example:
//
-// cc := momentum.NewCoppockCurve[float64]()
-//
-// result := cc.Compute(closings)
+// ```go
+// cc := momentum.NewCoppockCurve[float64]()
+// result := cc.Compute(closings)
+// ```
type CoppockCurve[T helper.Float] struct {📝 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.
Suggested change
| // CoppockCurve represents the configuration parameters for calculating the | |
| // Coppock Curve oscillator. The Coppock Curve is a momentum indicator | |
| // used to identify long-term buying opportunities in equity indices. | |
| // | |
| // Coppock Curve = WMA(ROC(14) + ROC(11), 10) | |
| // | |
| // Example: | |
| // | |
| // cc := momentum.NewCoppockCurve[float64]() | |
| // result := cc.Compute(closings) | |
| type CoppockCurve[T helper.Float] struct { | |
| // CoppockCurve represents the configuration parameters for calculating the | |
| // Coppock Curve oscillator. The Coppock Curve is a momentum indicator | |
| // used to identify long-term buying opportunities in equity indices. | |
| // | |
| // |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/coppock_curve.go` around lines 25 - 35, Update the doc comment above
the CoppockCurve type to use explicit fenced-code languages: wrap the formula
line with a plain text fence and the example with a ```go fenced block (i.e.,
replace the unlabeled triple-backtick blocks with "```text" for the formula and
"```go" for the example), ensure the example code lines begin with // inside the
comment and close the fences, then regenerate the README/docs so the labeled
fences appear in momentum/README.md; locate the comment immediately preceding
the CoppockCurve[T helper.Float] struct to make the edits.
| // | ||
| // cc := momentum.NewCoppockCurve[float64]() | ||
| // result := cc.Compute(closings) | ||
| type CoppockCurve[T helper.Float] struct { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Momentum indicators and their type constraints ==="
rg -n --type go 'type\s+\w+\[T helper\.(Number|Float|int|float|decimal)\]' momentum/ -g '!**/*_test.go' 2>/dev/null || true
echo ""
echo "=== Helper package type definitions ==="
rg -n --type go '(type Number|type Float)' helper/ 2>/dev/null || true
echo ""
echo "=== CoppockCurve struct definition ==="
rg -n --type go -A 5 'type CoppockCurve\[' momentum/coppock_curve.go
echo ""
echo "=== ROC definition in trend package (if exists) ==="
if [ -f "trend/roc.go" ]; then
rg -n --type go 'type Roc\[' trend/roc.go -A 3
else
echo "trend/roc.go not found"
fiRepository: cinar/indicator
Length of output: 1812
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Float and Number interface definitions ==="
rg -n --type go -A 5 'type (Float|Number) interface' helper/helper.goRepository: cinar/indicator
Length of output: 255
Change generic constraint from helper.Float to helper.Number.
The momentum guideline requires all indicators to be generic over helper.Number, but this uses helper.Float. Since Number = Integer | Float, using Float artificially restricts valid input types. This pattern appears in multiple momentum indicators (Rvi, Fisher, ConnorsRsi, PringsSpecialK) and should be corrected across the package for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/coppock_curve.go` at line 35, The generic constraint on CoppockCurve
is too narrow — change the type parameter from helper.Float to helper.Number so
the indicator accepts both Integer and Float inputs; update the struct
declaration for CoppockCurve[T helper.Number] and similarly replace helper.Float
with helper.Number in the other affected indicators (Rvi, Fisher, ConnorsRsi,
PringsSpecialK) to keep the package consistent.
Comment on lines
+74
to
+113
| func (c *CoppockCurve[T]) Compute(values <-chan T) <-chan T { | ||
| maxRocPeriod := c.RocPeriod1 | ||
| if c.RocPeriod2 > maxRocPeriod { | ||
| maxRocPeriod = c.RocPeriod2 | ||
| } | ||
|
|
||
| values = helper.Buffered(values, maxRocPeriod) | ||
| valuesSplice := helper.Duplicate(values, 2) | ||
|
|
||
| roc1 := trend.NewRocWithPeriod[T](c.RocPeriod1) | ||
| roc1Values := helper.MultiplyBy(roc1.Compute(valuesSplice[0]), 100) | ||
|
|
||
| roc2 := trend.NewRocWithPeriod[T](c.RocPeriod2) | ||
| roc2Values := helper.MultiplyBy(roc2.Compute(valuesSplice[1]), 100) | ||
|
|
||
| // Align ROC streams. Both Compute calls return streams that have already skipped their own IdlePeriod. | ||
| // To align them to maxRocPeriod, we skip the remaining difference. | ||
| if c.RocPeriod1 < maxRocPeriod { | ||
| roc1Values = helper.Skip(roc1Values, maxRocPeriod-c.RocPeriod1) | ||
| } | ||
| if c.RocPeriod2 < maxRocPeriod { | ||
| roc2Values = helper.Skip(roc2Values, maxRocPeriod-c.RocPeriod2) | ||
| } | ||
|
|
||
| sumRocs := helper.Add(roc1Values, roc2Values) | ||
|
|
||
| wma := trend.NewWmaWith[T](c.WmaPeriod) | ||
| return wma.Compute(sumRocs) | ||
| } | ||
|
|
||
| // IdlePeriod is the initial period that Coppock Curve won't yield any results. | ||
| func (c *CoppockCurve[T]) IdlePeriod() int { | ||
| maxRocPeriod := c.RocPeriod1 | ||
| if c.RocPeriod2 > maxRocPeriod { | ||
| maxRocPeriod = c.RocPeriod2 | ||
| } | ||
|
|
||
| // maxRocPeriod (from ROC) + (WmaPeriod - 1) (from WMA) | ||
| return maxRocPeriod + c.WmaPeriod - 1 | ||
| } |
Contributor
There was a problem hiding this comment.
Normalize periods inside Compute and IdlePeriod, not only in constructors.
NewCoppockCurveWithPeriods applies defaults, but Compute/IdlePeriod read raw fields directly. Since fields are public, zero/negative values via zero-value struct or later mutation can produce wrong stream alignment and runtime failures.
🛠️ Suggested fix (single normalization path)
+func (c *CoppockCurve[T]) normalizedPeriods() (rocPeriod1, rocPeriod2, wmaPeriod int) {
+ rocPeriod1 = c.RocPeriod1
+ if rocPeriod1 <= 0 {
+ rocPeriod1 = DefaultCoppockCurveRocPeriod1
+ }
+
+ rocPeriod2 = c.RocPeriod2
+ if rocPeriod2 <= 0 {
+ rocPeriod2 = DefaultCoppockCurveRocPeriod2
+ }
+
+ wmaPeriod = c.WmaPeriod
+ if wmaPeriod <= 0 {
+ wmaPeriod = DefaultCoppockCurveWmaPeriod
+ }
+
+ return
+}
+
// Compute function takes a channel of closings and computes the Coppock Curve.
func (c *CoppockCurve[T]) Compute(values <-chan T) <-chan T {
- maxRocPeriod := c.RocPeriod1
- if c.RocPeriod2 > maxRocPeriod {
- maxRocPeriod = c.RocPeriod2
+ rocPeriod1, rocPeriod2, wmaPeriod := c.normalizedPeriods()
+
+ maxRocPeriod := rocPeriod1
+ if rocPeriod2 > maxRocPeriod {
+ maxRocPeriod = rocPeriod2
}
values = helper.Buffered(values, maxRocPeriod)
valuesSplice := helper.Duplicate(values, 2)
- roc1 := trend.NewRocWithPeriod[T](c.RocPeriod1)
+ roc1 := trend.NewRocWithPeriod[T](rocPeriod1)
roc1Values := helper.MultiplyBy(roc1.Compute(valuesSplice[0]), 100)
- roc2 := trend.NewRocWithPeriod[T](c.RocPeriod2)
+ roc2 := trend.NewRocWithPeriod[T](rocPeriod2)
roc2Values := helper.MultiplyBy(roc2.Compute(valuesSplice[1]), 100)
// Align ROC streams. Both Compute calls return streams that have already skipped their own IdlePeriod.
// To align them to maxRocPeriod, we skip the remaining difference.
- if c.RocPeriod1 < maxRocPeriod {
- roc1Values = helper.Skip(roc1Values, maxRocPeriod-c.RocPeriod1)
+ if rocPeriod1 < maxRocPeriod {
+ roc1Values = helper.Skip(roc1Values, maxRocPeriod-rocPeriod1)
}
- if c.RocPeriod2 < maxRocPeriod {
- roc2Values = helper.Skip(roc2Values, maxRocPeriod-c.RocPeriod2)
+ if rocPeriod2 < maxRocPeriod {
+ roc2Values = helper.Skip(roc2Values, maxRocPeriod-rocPeriod2)
}
sumRocs := helper.Add(roc1Values, roc2Values)
- wma := trend.NewWmaWith[T](c.WmaPeriod)
+ wma := trend.NewWmaWith[T](wmaPeriod)
return wma.Compute(sumRocs)
}
// IdlePeriod is the initial period that Coppock Curve won't yield any results.
func (c *CoppockCurve[T]) IdlePeriod() int {
- maxRocPeriod := c.RocPeriod1
- if c.RocPeriod2 > maxRocPeriod {
- maxRocPeriod = c.RocPeriod2
+ rocPeriod1, rocPeriod2, wmaPeriod := c.normalizedPeriods()
+ maxRocPeriod := rocPeriod1
+ if rocPeriod2 > maxRocPeriod {
+ maxRocPeriod = rocPeriod2
}
// maxRocPeriod (from ROC) + (WmaPeriod - 1) (from WMA)
- return maxRocPeriod + c.WmaPeriod - 1
+ return maxRocPeriod + wmaPeriod - 1
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/coppock_curve.go` around lines 74 - 113, Compute and IdlePeriod
currently read the raw public fields (RocPeriod1, RocPeriod2, WmaPeriod) which
can be zero/invalid if the struct is zero-valued or mutated; normalize these
periods at the start of both Compute and IdlePeriod (e.g., derive local
maxRocPeriod, roc1Period, roc2Period, wmaPeriod from the fields but applying the
same defaulting/validation logic used in NewCoppockCurveWithPeriods) so all
downstream calls (helper.Buffered, trend.NewRocWithPeriod, trend.NewWmaWith,
helper.Skip and the IdlePeriod formula) use validated positive periods;
reference the symbols RocPeriod1, RocPeriod2, WmaPeriod, Compute, IdlePeriod,
and NewCoppockCurveWithPeriods when implementing the single normalization path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe Request
Coppock Curve is added.
Change Type
New indicator.
Summary by CodeRabbit
New Features
Tests