Add momentum indicator Martin Prings Special K #278
Conversation
WalkthroughAdds a new generic PringsSpecialK indicator type with constructor and Compute method that duplicates closing-price streams, computes 12 ROCs and 12 SMAs, aligns outputs using IdlePeriod-based skips, and emits a weighted aggregate; also adds unit tests covering initialization, idle gating, monotonicity, and constant-input behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant PSK as PringsSpecialK
participant Dup as helper.Duplicate
participant Roc as ROCs (12)
participant Sma as SMAs (12)
participant Align as helper.Skip / IdlePeriod
participant MulAdd as Multiply/Add pipeline
participant Out as Output
Caller->>PSK: Compute(closings)
PSK->>Dup: duplicate closings into 12 streams
Dup-->>Roc: feed each ROC(period)
Roc-->>Sma: each ROC -> corresponding SMA(period)
Sma-->>Align: SMA streams (have IdlePeriod)
Align-->>MulAdd: aligned SMA outputs (skipped as needed)
MulAdd->>MulAdd: apply weights [1,2,3,4,...]
MulAdd-->>Out: emit weighted sum values
Out-->>Caller: <-chan T
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #278 +/- ##
==========================================
+ Coverage 91.71% 91.81% +0.10%
==========================================
Files 182 183 +1
Lines 6587 6672 +85
==========================================
+ Hits 6041 6126 +85
Misses 486 486
Partials 60 60 ☔ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
momentum/prings_special_k_test.go (1)
48-67: Avoid storing outputs; count them to reduce memory.You only need the length. Counting is cheaper and avoids holding the slice.
- var got []float64 - for v := range out { - got = append(got, v) - } - - return len(got) + n := 0 + for range out { + n++ + } + return nmomentum/prings_special_k.go (3)
86-99: Idle alignment is sound; consider defensive max(0, …) for readability.The maxIdle-based skips correctly align series. Using an explicit max(0, x) would document intent if formulas ever change.
Example:
+safe := func(v int) int { if v < 0 { return 0 }; return v } -sma10 = helper.Skip(sma10, maxIdle-p.Sma10.IdlePeriod()-p.Roc10.IdlePeriod()) +sm a10 = helper.Skip(sma10, safe(maxIdle-p.Sma10.IdlePeriod()-p.Roc10.IdlePeriod()))(Apply similarly to the other Skip calls.)
99-99: Remove commented-out code or replace with an explanatory comment.Dead code comment can be confusing.
- //sma530 = helper.Skip(sma530, p.Sma530.IdlePeriod()-p.Sma530.IdlePeriod()) + // sma530 is the reference (slowest path); no skip needed.
101-114: Weights pipeline is fine; optional simplification to reduce channel fan-in depth.Chaining 12 Add stages increases goroutine/channel overhead. A small fold can keep the pipeline shallow.
Example:
- p0 := helper.MultiplyBy(sma10, 1) - p1 := helper.Add(p0, helper.MultiplyBy(sma15, 2)) - ... - p11 := helper.Add(p10, helper.MultiplyBy(sma530, 4)) - return p11 + terms := []<-chan T{ + helper.MultiplyBy(sma10, 1), + helper.MultiplyBy(sma15, 2), + helper.MultiplyBy(sma20, 3), + helper.MultiplyBy(sma30, 4), + helper.MultiplyBy(sma40, 1), + helper.MultiplyBy(sma65, 2), + helper.MultiplyBy(sma75, 3), + helper.MultiplyBy(sma100, 4), + helper.MultiplyBy(sma195, 1), + helper.MultiplyBy(sma265, 2), + helper.MultiplyBy(sma390, 3), + helper.MultiplyBy(sma530, 4), + } + sum := terms[0] + for i := 1; i < len(terms); i++ { + sum = helper.Add(sum, terms[i]) + } + return sum
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
momentum/prings_special_k.go(1 hunks)momentum/prings_special_k_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
momentum/prings_special_k_test.go (2)
momentum/prings_special_k.go (1)
NewPringsSpecialK(33-57)trend/roc.go (1)
NewRocWithPeriod(32-39)
momentum/prings_special_k.go (2)
trend/roc.go (2)
Roc(21-24)NewRocWithPeriod(32-39)helper/skip.go (1)
Skip(15-30)
🪛 GitHub Actions: Build
momentum/prings_special_k.go
[error] 8-8: Revive lint error: exported type 'PringsSpecialK' should have a comment or be unexported. Command: go run github.com/mgechev/[email protected] -config=revive.toml $INDICATOR_BASE
🔇 Additional comments (4)
momentum/prings_special_k_test.go (3)
11-23: LGTM: constructor initializes all ROC/SMA components.All expected pointers are non-nil. Good coverage for the initialization path.
69-85: LGTM: idle threshold check is clear and robust.Good use of IdlePeriod composition to validate gating behavior.
87-105: LGTM: constant-input yields zero output.Solid invariant test for ROC→SMA→weighted-sum pipeline.
momentum/prings_special_k.go (1)
59-70: Input fan-out to ROC streams looks correct.Eight duplicates feed the eight ROC periods. No issues spotted.
| func TestPringsSpecialKIsDecreasing(t *testing.T) { | ||
| f := 0.1 | ||
| r := make([]float64, 0) | ||
| for i := 0; i < 800; i++ { | ||
| r = append(r, f) | ||
| f += 0.01 | ||
| } | ||
| cl := helper.SliceToChan(r) | ||
| sp := NewPringsSpecialK[float64]() | ||
| rs := sp.Compute(cl) | ||
| p := math.NaN() | ||
| upwardCount := 0 | ||
| for v := range rs { | ||
| if !math.IsNaN(p) && p <= v { | ||
| upwardCount++ | ||
| } | ||
| p = v | ||
| } | ||
| if upwardCount != 0 { | ||
| t.Error("Prings Special K should be decreasing on increasing values") | ||
| } | ||
| } |
There was a problem hiding this comment.
Test expectation is inverted; Special K on rising prices should not be strictly decreasing.
This assertion will be flaky/wrong. For steadily rising inputs, momentum should rise then level off; enforcing “strictly decreasing” contradicts that.
Apply this diff to assert non-decreasing (allows plateaus, forbids drops):
-func TestPringsSpecialKIsDecreasing(t *testing.T) {
+func TestPringsSpecialKIsNonDecreasingOnRisingPrices(t *testing.T) {
@@
- upwardCount := 0
+ downwardCount := 0
for v := range rs {
- if !math.IsNaN(p) && p <= v {
- upwardCount++
+ if !math.IsNaN(p) && v < p {
+ downwardCount++
}
p = v
}
- if upwardCount != 0 {
- t.Error("Prings Special K should be decreasing on increasing values")
+ if downwardCount != 0 {
+ t.Error("Prings Special K should not decrease on steadily increasing input")
}
}📝 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.
| func TestPringsSpecialKIsDecreasing(t *testing.T) { | |
| f := 0.1 | |
| r := make([]float64, 0) | |
| for i := 0; i < 800; i++ { | |
| r = append(r, f) | |
| f += 0.01 | |
| } | |
| cl := helper.SliceToChan(r) | |
| sp := NewPringsSpecialK[float64]() | |
| rs := sp.Compute(cl) | |
| p := math.NaN() | |
| upwardCount := 0 | |
| for v := range rs { | |
| if !math.IsNaN(p) && p <= v { | |
| upwardCount++ | |
| } | |
| p = v | |
| } | |
| if upwardCount != 0 { | |
| t.Error("Prings Special K should be decreasing on increasing values") | |
| } | |
| } | |
| func TestPringsSpecialKIsNonDecreasingOnRisingPrices(t *testing.T) { | |
| f := 0.1 | |
| r := make([]float64, 0) | |
| for i := 0; i < 800; i++ { | |
| r = append(r, f) | |
| f += 0.01 | |
| } | |
| cl := helper.SliceToChan(r) | |
| sp := NewPringsSpecialK[float64]() | |
| rs := sp.Compute(cl) | |
| p := math.NaN() | |
| downwardCount := 0 | |
| for v := range rs { | |
| if !math.IsNaN(p) && v < p { | |
| downwardCount++ | |
| } | |
| p = v | |
| } | |
| if downwardCount != 0 { | |
| t.Error("Prings Special K should not decrease on steadily increasing input") | |
| } | |
| } |
🤖 Prompt for AI Agents
In momentum/prings_special_k_test.go around lines 25 to 46 the test inverts the
expectation: for steadily rising inputs Special K should not strictly decrease;
update the check to detect any decreases (p > v) rather than increases, count
decreases (downwardCount) during the iteration and fail if that count is
non‑zero, and update the test error message to state that Prings Special K
should be non‑decreasing (allowing plateaus) on increasing values.
There was a problem hiding this comment.
This is not correct. Prings Special K is made from ROC. For example, a ROC 10 feeded with increasing values, i.e. 1, 2, 3, ... will calculate the following results, starting from value 11:
(11 - 1) / 1 => 10, next is (12 - 2) / 2 => 5, next is (13 - 3) / 3 = 3.33, ... so I get all the ROCs in the calculation decreasing. Even with an SMA the final result is still decreasing.
There was a problem hiding this comment.
🧩 Analysis chain
@hwde You're absolutely correct, and I apologize for the confusion. Thank you for the detailed explanation!
Looking at the ROC calculation, you're right that with linearly increasing values, the ROC will indeed be decreasing. Your math is spot-on:
- ROC formula:
(current - previous) / previous * 100 - With sequence 1, 2, 3, 4, ... the ROC(10) results are:
- (11-1)/1 = 10 = 1000%
- (12-2)/2 = 5 = 500%
- (13-3)/3 = 3.33 = 333%
- And so on, decreasing
Since Prings Special K is a weighted combination of multiple ROC periods smoothed with SMAs, and all the underlying ROCs are decreasing with linearly increasing input, the final result will indeed be decreasing.
The test TestPringsSpecialKIsDecreasing is correctly written and should pass. My original suggestion was based on an incorrect understanding of how ROC behaves with steadily increasing values.
🏁 Script executed:
# Let's examine the ROC (Rate of Change) implementation to understand the formula
ast-grep --pattern 'func $_($$$) $_' | grep -i roc
Length of output: 3459
🏁 Script executed:
# Find ROC implementation files
fd -e go roc
Length of output: 30
🏁 Script executed:
# Look at the ROC implementation to understand the calculation
rg -A 10 -B 5 "type Roc" --type go
Length of output: 702
🏁 Script executed:
# Find the ROC computation logic
rg -A 15 "func.*Roc.*Compute" --type go
Length of output: 510
🏁 Script executed:
# Look at trend package ROC implementation
cat trend/roc.go
Length of output: 1639
@hwde You're absolutely correct, and I apologize for the confusion. Thank you for the detailed explanation!
Looking at the ROC implementation in the codebase, I can now confirm your analysis is spot-on. The ROC formula is indeed:
ROC = (Current Price - Price n periods ago) / Price n periods ago
Your math is perfect:
- With sequence 1, 2, 3, 4, ... the ROC(10) results are:
- (11-1)/1 = 10
- (12-2)/2 = 5
- (13-3)/3 = 3.33
- And so on, decreasing
Since Prings Special K is a weighted combination of multiple ROC periods (10, 15, 50, 65, 75, 100, 130, 195) smoothed with SMAs, and all the underlying ROCs are decreasing with linearly increasing input, the final result will indeed be decreasing.
The test TestPringsSpecialKIsDecreasing is correctly written and should pass. My original suggestion was based on an incorrect understanding of how ROC behaves with steadily increasing values. The key insight is that with constant absolute increases, the percentage change (ROC) decreases because the denominator keeps growing while the numerator stays constant.
✏️ Learnings added
Learnt from: hwde
PR: cinar/indicator#278
File: momentum/prings_special_k_test.go:25-46
Timestamp: 2025-09-11T18:49:29.832Z
Learning: Prings Special K indicator decreases on linearly increasing input values because it's based on ROC (Rate of Change) calculations. With linearly increasing values like 1,2,3,..., ROC percentages decrease over time since the absolute change remains constant while the base value increases, making the test expectation of decreasing values correct.
Learnt from: hwde
PR: cinar/indicator#278
File: momentum/prings_special_k_test.go:25-46
Timestamp: 2025-09-11T18:49:29.832Z
Learning: Prings Special K indicator decreases on linearly increasing input values because it's based on ROC (Rate of Change) calculations. With linearly increasing values like 1,2,3,..., ROC percentages decrease over time since the absolute change remains constant while the base value increases, making the test expectation of decreasing values correct.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
momentum/prings_special_k.go (3)
114-125: Drop no-op MultiplyBy(..., 1) to reduce goroutines.Avoid spawning unnecessary workers for weight 1 terms.
Apply:
- p0 := helper.MultiplyBy(sma10Roc10, 1) + p0 := sma10Roc10 p1 := helper.Add(p0, helper.MultiplyBy(sma10Roc15, 2)) p2 := helper.Add(p1, helper.MultiplyBy(sma10Roc20, 3)) p3 := helper.Add(p2, helper.MultiplyBy(sma15Roc30, 4)) - p4 := helper.Add(p3, helper.MultiplyBy(sma50Roc40, 1)) + p4 := helper.Add(p3, sma50Roc40) p5 := helper.Add(p4, helper.MultiplyBy(sma65Roc65, 2)) p6 := helper.Add(p5, helper.MultiplyBy(sma75Roc75, 3)) p7 := helper.Add(p6, helper.MultiplyBy(sma100Roc100, 4)) - p8 := helper.Add(p7, helper.MultiplyBy(sma130Roc195, 1)) + p8 := helper.Add(p7, sma130Roc195) p9 := helper.Add(p8, helper.MultiplyBy(sma130Roc265, 2)) p10 := helper.Add(p9, helper.MultiplyBy(sma130Roc390, 3)) p11 := helper.Add(p10, helper.MultiplyBy(sma195Roc530, 4))
69-71: Expose IdlePeriod for consistency with other indicators.Handy for callers/tests; documents required samples for first output.
Apply:
} +// IdlePeriod returns the number of input samples required before the first output. +func (p *PringsSpecialK[T]) IdlePeriod() int { + return p.Sma195Roc530.IdlePeriod() + p.Roc530.IdlePeriod() +} + func (p *PringsSpecialK[T]) Compute(closings <-chan T) <-chan T {
100-113: Optional: clamp skip to non-negative for defensive safety.Today this difference is non-negative by construction; a clamp avoids accidental negatives if internals change.
Example:
- sma10Roc10 = helper.Skip(sma10Roc10, maxIdle-p.Sma10Roc10.IdlePeriod()-p.Roc10.IdlePeriod()) + if d := maxIdle - p.Sma10Roc10.IdlePeriod() - p.Roc10.IdlePeriod(); d > 0 { + sma10Roc10 = helper.Skip(sma10Roc10, d) + }(Repeat pattern for the other streams.)
momentum/prings_special_k_test.go (2)
71-87: Make threshold derivation mirror production mapping (readability).Use SMA(195)+ROC(530) (as in Compute) rather than SMA(530)+ROC(195). Sums match today, but mirroring the implementation aids maintainability. If you add IdlePeriod() on the indicator, prefer using that directly.
Option A (mirror mapping now):
- sma530 := trend.NewSmaWithPeriod[float64](530) - roc195 := trend.NewRocWithPeriod[float64](195) - minimumRequiredInputLength := sma530.IdlePeriod() + roc195.IdlePeriod() + 1 + sma195 := trend.NewSmaWithPeriod[float64](195) + roc530 := trend.NewRocWithPeriod[float64](530) + minimumRequiredInputLength := sma195.IdlePeriod() + roc530.IdlePeriod() + 1Option B (after adding PringsSpecialK.IdlePeriod):
- sma195 := trend.NewSmaWithPeriod[float64](195) - roc530 := trend.NewRocWithPeriod[float64](530) - minimumRequiredInputLength := sma195.IdlePeriod() + roc530.IdlePeriod() + 1 + psk := NewPringsSpecialK[float64]() + minimumRequiredInputLength := psk.IdlePeriod() + 1
89-107: Consider tolerant equality for floats (ultra-conservative).Zero is expected here; if the pipeline ever introduces rounding, use an epsilon check.
Example:
- if v != 0.0 { + if math.Abs(v) > 1e-12 {(Import math if not already in scope.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
momentum/prings_special_k.go(1 hunks)momentum/prings_special_k_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: hwde
PR: cinar/indicator#278
File: momentum/prings_special_k_test.go:25-46
Timestamp: 2025-09-11T18:49:29.832Z
Learning: Prings Special K indicator decreases on linearly increasing input values because it's based on ROC (Rate of Change) calculations. With linearly increasing values like 1,2,3,..., ROC percentages decrease over time since the absolute change remains constant while the base value increases, making the test expectation of decreasing values correct.
Learnt from: hwde
PR: cinar/indicator#278
File: momentum/prings_special_k_test.go:25-46
Timestamp: 2025-09-11T18:49:29.832Z
Learning: Prings Special K indicator decreases on linearly increasing input values because it's based on ROC (Rate of Change) calculations. With linearly increasing values like 1,2,3,..., ROC percentages decrease over time since the absolute change remains constant while the base value increases, making the test expectation of decreasing values correct.
📚 Learning: 2025-09-11T18:49:29.832Z
Learnt from: hwde
PR: cinar/indicator#278
File: momentum/prings_special_k_test.go:25-46
Timestamp: 2025-09-11T18:49:29.832Z
Learning: Prings Special K indicator decreases on linearly increasing input values because it's based on ROC (Rate of Change) calculations. With linearly increasing values like 1,2,3,..., ROC percentages decrease over time since the absolute change remains constant while the base value increases, making the test expectation of decreasing values correct.
Applied to files:
momentum/prings_special_k_test.go
🧬 Code graph analysis (2)
momentum/prings_special_k_test.go (2)
momentum/prings_special_k.go (1)
NewPringsSpecialK(41-69)trend/roc.go (1)
NewRocWithPeriod(32-39)
momentum/prings_special_k.go (5)
trend/roc.go (2)
Roc(21-24)NewRocWithPeriod(32-39)helper/skip.go (1)
Skip(15-30)momentum/stochastic_oscillator.go (1)
Compute(52-79)momentum/chaikin_oscillator_test.go (1)
TestChaikinOscillator(14-49)momentum/ichimoku_cloud.go (1)
Compute(78-136)
🪛 GitHub Actions: Build
momentum/prings_special_k.go
[error] 71-71: staticcheck: exported method PringsSpecialK.Compute should have comment or be unexported
🔇 Additional comments (1)
momentum/prings_special_k_test.go (1)
27-48: Decreasing-behavior test looks correct.Agree with the ROC-based rationale; the assertion is appropriate.
| } | ||
| } | ||
|
|
||
| func (p *PringsSpecialK[T]) Compute(closings <-chan T) <-chan T { |
There was a problem hiding this comment.
Fix build: add doc comment for exported Compute.
Staticcheck failure: “exported method PringsSpecialK.Compute should have comment or be unexported.” Add a doc comment that starts with “Compute”.
Apply:
- func (p *PringsSpecialK[T]) Compute(closings <-chan T) <-chan T {
+// Compute returns Martin Pring's Special K stream, defined as the weighted sum:
+// 1*SMA10(ROC10) + 2*SMA10(ROC15) + 3*SMA10(ROC20) + 4*SMA15(ROC30) +
+// 1*SMA50(ROC40) + 2*SMA65(ROC65) + 3*SMA75(ROC75) + 4*SMA100(ROC100) +
+// 1*SMA130(ROC195) + 2*SMA130(ROC265) + 3*SMA130(ROC390) + 4*SMA195(ROC530).
+// Outputs are time-aligned by skipping early samples to match the slowest path.
+func (p *PringsSpecialK[T]) Compute(closings <-chan T) <-chan T {📝 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.
| func (p *PringsSpecialK[T]) Compute(closings <-chan T) <-chan T { | |
| // Compute returns Martin Pring's Special K stream, defined as the weighted sum: | |
| // 1*SMA10(ROC10) + 2*SMA10(ROC15) + 3*SMA10(ROC20) + 4*SMA15(ROC30) + | |
| // 1*SMA50(ROC40) + 2*SMA65(ROC65) + 3*SMA75(ROC75) + 4*SMA100(ROC100) + | |
| // 1*SMA130(ROC195) + 2*SMA130(ROC265) + 3*SMA130(ROC390) + 4*SMA195(ROC530). | |
| // Outputs are time-aligned by skipping early samples to match the slowest path. | |
| func (p *PringsSpecialK[T]) Compute(closings <-chan T) <-chan T { |
🧰 Tools
🪛 GitHub Actions: Build
[error] 71-71: staticcheck: exported method PringsSpecialK.Compute should have comment or be unexported
🤖 Prompt for AI Agents
In momentum/prings_special_k.go around line 71, the exported method
PringsSpecialK.Compute lacks a doc comment which causes a Staticcheck failure;
add a Go doc comment immediately above the method that begins with "Compute" and
succinctly describes what the method does, its parameters (closings <-chan T)
and its return value (<-chan T), keeping the comment one or two sentences and
starting with the word "Compute".
3 participants
Describe Request
Please describe your request.
Fixed #260
Change Type
What is the type of this change.
Summary by CodeRabbit
New Features
Tests