Klinger Volume Oscillator (KVO) is added.#327
Conversation
📝 WalkthroughWalkthroughThis pull request implements the Klinger Volume Oscillator (KVO), a volume-based technical indicator that tracks money flow trends through EMA differences. It introduces a new helper function Operate5 for five-channel operations, refactors RVI and KST stream handling for signal alignment, and adds configuration files along with comprehensive tests and documentation updates. Changes
Sequence DiagramsequenceDiagram
participant Input as Input Streams<br/>(highs, lows, volumes)
participant VF as VF Computation<br/>(Volume Force)
participant ShortEMA as Short EMA<br/>(Period 34)
participant LongEMA as Long EMA<br/>(Period 55)
participant KVO as KVO Difference<br/>(Short - Long)
participant SignalEMA as Signal EMA<br/>(Period 13)
participant Output as Output<br/>(KVO + Signal)
Input->>VF: Duplicate streams with shift
VF->>VF: Compute Trend (high/low comparison)<br/>VF = Volume × Trend
VF->>ShortEMA: Stream to short period EMA
VF->>LongEMA: Stream to long period EMA
ShortEMA->>ShortEMA: Apply EMA(34)
LongEMA->>LongEMA: Apply EMA(55)
ShortEMA->>KVO: Align via idle period
LongEMA->>KVO: Align via idle period
KVO->>KVO: Compute difference
KVO->>SignalEMA: Stream to signal EMA
SignalEMA->>SignalEMA: Apply EMA(13)
KVO->>Output: Emit KVO with skip
SignalEMA->>Output: Emit Signal result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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❌ Patch coverage is
❌ Your project status has failed because the head coverage (88.99%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #327 +/- ##
==========================================
- Coverage 88.99% 88.99% -0.01%
==========================================
Files 205 207 +2
Lines 5761 5822 +61
==========================================
+ Hits 5127 5181 +54
- Misses 563 568 +5
- Partials 71 73 +2 ☔ 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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@helper/README.md`:
- Around line 840-855: The README's Operate5 documentation uses fenced code
blocks without language labels (triggering markdownlint MD040); update the three
fenced blocks around the Operate5 section — the function signature block and the
example block(s) that contain the code sample for Operate5 — to use the "go"
language tag (replace ``` with ```go) so the blocks specify language and satisfy
the linter while preserving the existing code and formatting.
In `@momentum/rvi_test.go`:
- Around line 41-44: The loop that collects from rviResult and signalResult
should guard the paired receive from signalResult with the comma-ok form to
detect closed channels: in the loop that iterates over rviResult (referencing
rviResult, rviSlice, signalResult, signalSlice) replace the direct
<-signalResult receive with a two-value receive (val, ok := <-signalResult) and,
if !ok, stop the loop or handle the unexpected early close (e.g., break and fail
the test), otherwise append val to signalSlice; ensure rvi values are still
appended to rviSlice only when the paired signal value was successfully
received.
In `@trend/kst_test.go`:
- Around line 30-33: The test currently pairs values from actualKst and
actualSignal using single-value receives which masks premature channel closure;
update both loops that iterate over actualKst (the block using variables
kstSlice and signalSlice and the similar block at lines ~99-102) to perform
two-value receives from actualSignal (s, ok := <-actualSignal), check ok, and
fail the test (e.g., t.Fatalf or equivalent) if ok is false before appending, so
a closed signal channel is detected rather than silently using zero-values.
In `@volume/kvo_test.go`:
- Around line 37-53: The current test reads from kvoResult and signalResult
together (looping over kvoResult and consuming signalResult inside), which can
hang on mismatched channel lengths and uses manual checks; change to drain each
channel independently (e.g., range kvoResult to append to kvoSlice until closed,
then range signalResult to append to signalSlice) so length mismatches fail
cleanly, then replace manual NaN/Inf and length checks with the project test
helpers: use helper.RoundDigits(...) when comparing float64 values and assert
equality with helper.CheckEquals(...) for expected counts/values; update the
assertions around kvoSlice and signalSlice and any float comparisons to call
helper.RoundDigits before passing values to helper.CheckEquals.
In `@volume/README.md`:
- Around line 395-409: The fenced code blocks for the KVO formula and the Go
example are missing language identifiers; update the formula block to use a
neutral identifier (e.g., ```text) and the example block to use the Go
identifier (```go) so markdownlint MD040 is satisfied; specifically edit the
README's KVO section containing the formula block and the snippet that creates
NewKvo[float64]() and calls Compute(highs, lows, volumes) to add the appropriate
fenced-language tags.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.geminiignore.gitignoreAGENTS.mdREADME.mdhelper/README.mdhelper/operate5.gohelper/operate5_test.gomomentum/README.mdmomentum/rvi.gomomentum/rvi_test.gotrend/README.mdtrend/kst.gotrend/kst_test.govolume/README.mdvolume/kvo.govolume/kvo_test.go
💤 Files with no reviewable changes (1)
- AGENTS.md
| <a name="Operate5"></a> | ||
| ## func [Operate5](<https://github.com/cinar/indicator/blob/master/helper/operate5.go#L15-L22>) | ||
|
|
||
| ```go | ||
| func Operate5[A any, B any, C any, D any, E any, R any](ac <-chan A, bc <-chan B, cc <-chan C, dc <-chan D, ec <-chan E, o func(A, B, C, D, E) R) <-chan R | ||
| ``` | ||
|
|
||
| Operate5 applies the provided operate function to corresponding values from five numeric input channels and sends the resulting values to an output channel. | ||
|
|
||
| Example: | ||
|
|
||
| ``` | ||
| result := helper.Operate5(ac, bc, cc, dc, ec, func(a, b, c, d, e int) int { | ||
| return a + b + c + d + e | ||
| }) | ||
| ``` |
There was a problem hiding this comment.
Specify language on the new fenced code blocks.
The newly added Operate5 section includes fenced blocks without explicit language, which triggers markdownlint MD040.
📝 Suggested fix
-```
+```go
result := helper.Operate5(ac, bc, cc, dc, ec, func(a, b, c, d, e int) int {
return a + b + c + d + e
})
-```
+```🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 840-840: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 847-847: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@helper/README.md` around lines 840 - 855, The README's Operate5 documentation
uses fenced code blocks without language labels (triggering markdownlint MD040);
update the three fenced blocks around the Operate5 section — the function
signature block and the example block(s) that contain the code sample for
Operate5 — to use the "go" language tag (replace ``` with ```go) so the blocks
specify language and satisfy the linter while preserving the existing code and
formatting.
| for v := range rviResult { | ||
| rviSlice = append(rviSlice, v) | ||
| signalSlice = append(signalSlice, <-signalResult) | ||
| } |
There was a problem hiding this comment.
Guard paired channel reads with ok checks.
Using <-signalResult directly can hide early channel closure by appending zero values, which may let regressions slip through.
✅ Suggested fix
- for v := range rviResult {
- rviSlice = append(rviSlice, v)
- signalSlice = append(signalSlice, <-signalResult)
- }
+ for v := range rviResult {
+ signal, ok := <-signalResult
+ if !ok {
+ t.Fatalf("signal result closed before rvi at index %d", len(rviSlice))
+ }
+
+ rviSlice = append(rviSlice, v)
+ signalSlice = append(signalSlice, signal)
+ }
+
+ if _, ok := <-signalResult; ok {
+ t.Fatal("signal produced extra values beyond rvi")
+ }#!/bin/bash
# Verify one-value receive usage in the paired RVI collection loop.
rg -nP --type=go 'signalSlice\s*=\s*append\(\s*signalSlice,\s*<-\s*signalResult\s*\)' momentum/rvi_test.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@momentum/rvi_test.go` around lines 41 - 44, The loop that collects from
rviResult and signalResult should guard the paired receive from signalResult
with the comma-ok form to detect closed channels: in the loop that iterates over
rviResult (referencing rviResult, rviSlice, signalResult, signalSlice) replace
the direct <-signalResult receive with a two-value receive (val, ok :=
<-signalResult) and, if !ok, stop the loop or handle the unexpected early close
(e.g., break and fail the test), otherwise append val to signalSlice; ensure rvi
values are still appended to rviSlice only when the paired signal value was
successfully received.
| for v := range actualKst { | ||
| kstSlice = append(kstSlice, v) | ||
| signalSlice = append(signalSlice, <-actualSignal) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n trend/kst_test.go | head -50Repository: cinar/indicator
Length of output: 1608
🏁 Script executed:
sed -n '25,110p' trend/kst_test.go | cat -nRepository: cinar/indicator
Length of output: 2852
🏁 Script executed:
# Also check for the pattern mentioned in the review
rg -nP --type=go 'signalSlice\s*=\s*append\(\s*signalSlice,\s*<-\s*actualSignal\s*\)' trend/kst_test.goRepository: cinar/indicator
Length of output: 169
🏁 Script executed:
cat trend/kst.go | head -100Repository: cinar/indicator
Length of output: 2798
🏁 Script executed:
# Find the Compute method for KST
ast-grep --pattern $'func ($_ *Kst[$_]) Compute($_ <-chan $_) ($_ <-chan $_, $_ <-chan $_) {
$$$
}'Repository: cinar/indicator
Length of output: 2317
🏁 Script executed:
# Alternative: search for Compute in kst.go
rg -nA 30 'func.*Compute' trend/kst.go | head -80Repository: cinar/indicator
Length of output: 969
🏁 Script executed:
# Check the Skip helper implementation
rg -nA 10 'func Skip' helper/ | head -30Repository: cinar/indicator
Length of output: 1535
🏁 Script executed:
# Also check how SMA Compute works to understand channel length contracts
fd 'sma' trend/ --type f -x basename {}Repository: cinar/indicator
Length of output: 94
🏁 Script executed:
# Check SMA Compute to verify channel length contract
rg -nA 20 'func.*Sma.*Compute' trend/sma.go | head -50Repository: cinar/indicator
Length of output: 556
🏁 Script executed:
# Check the Duplicate helper function
rg -nA 20 'func Duplicate' helper/ | head -50Repository: cinar/indicator
Length of output: 3420
🏁 Script executed:
# Check the Apply helper function used in SMA
rg -nA 15 'func Apply' helper/ | head -40Repository: cinar/indicator
Length of output: 2654
🏁 Script executed:
# Let's verify the exact lines referenced in the review
sed -n '30,33p' trend/kst_test.go | cat -n
sed -n '99,102p' trend/kst_test.go | cat -nRepository: cinar/indicator
Length of output: 346
🏁 Script executed:
# Check if there are additional test functions that also consume these channels
grep -n "for.*range actual" trend/kst_test.goRepository: cinar/indicator
Length of output: 120
🏁 Script executed:
# Let's trace through the channel operations more carefully
# Check if there are other test files using paired channels to see patterns
fd '_test.go' trend/ -x basename {} | head -10Repository: cinar/indicator
Length of output: 210
🏁 Script executed:
# Check if there's a test pattern used in similar paired-channel indicators
grep -l "actualSignal" trend/*_test.goRepository: cinar/indicator
Length of output: 95
🏁 Script executed:
# Verify: does Skip maintain length parity with the original?
# Check what Skip does with count parameter
cat helper/skip.go | head -50Repository: cinar/indicator
Length of output: 659
🏁 Script executed:
# Let's check if MACD has the same pattern and see how it handles the channels
sed -n '1,150p' trend/macd_test.go | grep -A 5 "for.*range"Repository: cinar/indicator
Length of output: 245
🏁 Script executed:
# Let's examine the actual channel flow and understand the length contract
# Check the MovingSum implementation to understand how many values it outputs
rg -nA 25 'func.*MovingSum.*Compute' trend/Repository: cinar/indicator
Length of output: 4751
🏁 Script executed:
# Let me trace through the channel length logic more carefully
# Check what values MovingSum actually outputs
sed -n '34,46p' trend/moving_sum.goRepository: cinar/indicator
Length of output: 332
🏁 Script executed:
# Check the Operate helper function
rg -nA 20 'func Operate' helper/ | head -40Repository: cinar/indicator
Length of output: 1500
🏁 Script executed:
# Verify the channel flow in KST one more time by checking line 164-169
sed -n '164,169p' trend/kst.goRepository: cinar/indicator
Length of output: 257
🏁 Script executed:
# Check the Shift helper to understand channel length preservation
rg -nA 20 'func Shift' helper/ | head -40Repository: cinar/indicator
Length of output: 2698
🏁 Script executed:
# Verify SMA output length matches input length by checking Apply
# Apply should preserve length since it just transforms values
sed -n '44,51p' trend/sma.goRepository: cinar/indicator
Length of output: 242
🏁 Script executed:
# Now I need to verify the exact channel output lengths
# Let's trace through: what is maxIdle in KST?
sed -n '130,151p' trend/kst.goRepository: cinar/indicator
Length of output: 542
🏁 Script executed:
# And check the default periods
sed -n '10,45p' trend/kst.go | grep 'Default'Repository: cinar/indicator
Length of output: 888
🏁 Script executed:
# Let's understand ROC output length
rg -nA 20 'func.*Roc.*Compute' trend/roc.go | head -40Repository: cinar/indicator
Length of output: 483
🏁 Script executed:
# Also need to understand how many values the test data has and what the expected output is
sed -n '15,45p' trend/kst_test.goRepository: cinar/indicator
Length of output: 851
🏁 Script executed:
# Check the Add helper to confirm it maintains channel length parity
rg -nA 20 'func Add' helper/ | head -50Repository: cinar/indicator
Length of output: 3488
🏁 Script executed:
# Let me also check if there's a test that validates the actual output lengths
tail -30 trend/kst_test.goRepository: cinar/indicator
Length of output: 781
Use two-value receives to detect premature channel closure when pairing KST and signal streams.
Both loops receive from actualSignal without checking the ok flag. If the signal channel closes early (before the KST channel), the receive silently yields zero-values instead of erroring. Use two-value receives with explicit length validation:
Suggested fix pattern (apply to both loops)
- for v := range actualKst {
- kstSlice = append(kstSlice, v)
- signalSlice = append(signalSlice, <-actualSignal)
- }
+ for v := range actualKst {
+ signal, ok := <-actualSignal
+ if !ok {
+ t.Fatalf("signal closed before kst at index %d", len(kstSlice))
+ }
+
+ kstSlice = append(kstSlice, v)
+ signalSlice = append(signalSlice, signal)
+ }
+
+ if _, ok := <-actualSignal; ok {
+ t.Fatal("signal produced extra values beyond kst")
+ }Applies to lines 30-33 and 99-102.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@trend/kst_test.go` around lines 30 - 33, The test currently pairs values from
actualKst and actualSignal using single-value receives which masks premature
channel closure; update both loops that iterate over actualKst (the block using
variables kstSlice and signalSlice and the similar block at lines ~99-102) to
perform two-value receives from actualSignal (s, ok := <-actualSignal), check
ok, and fail the test (e.g., t.Fatalf or equivalent) if ok is false before
appending, so a closed signal channel is detected rather than silently using
zero-values.
| for v := range kvoResult { | ||
| kvoSlice = append(kvoSlice, v) | ||
| signalSlice = append(signalSlice, <-signalResult) | ||
| } | ||
|
|
||
| if len(kvoSlice) == 0 { | ||
| t.Fatal("KVO produced no output") | ||
| } | ||
|
|
||
| for i, v := range kvoSlice { | ||
| if math.IsNaN(v) || math.IsInf(v, 0) { | ||
| t.Fatalf("KVO at index %d is NaN or Inf: %v", i, v) | ||
| } | ||
| } | ||
|
|
||
| t.Logf("KVO: %d values, Signal: %d values", len(kvoSlice), len(signalSlice)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the kvo_test.go file and verify helper usage
fd -type f "kvo_test.go" 2>/dev/null | head -5Repository: cinar/indicator
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check for helper function usage in kvo_test.go
rg -n 'helper\.(CheckEquals|RoundDigits|ReadFromCsvFile)' --type go 2>/dev/null || echo "No helper functions found in pattern search"Repository: cinar/indicator
Length of output: 46374
🏁 Script executed:
#!/bin/bash
# Get the full content of the test file to understand its structure
fd -type f -name "kvo_test.go" -exec wc -l {} \;Repository: cinar/indicator
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Read the kvo_test.go file to see the actual implementation
fd -type f -name "kvo_test.go" -exec cat -n {} \;Repository: cinar/indicator
Length of output: 229
🏁 Script executed:
# Read the kvo_test.go file to see the actual implementation
find . -type f -name "kvo_test.go" -o -name "kvo.go" | head -10Repository: cinar/indicator
Length of output: 95
🏁 Script executed:
# Check for kvo files more directly
ls -la volume/ | grep -i kvoRepository: cinar/indicator
Length of output: 179
🏁 Script executed:
# Read kvo_test.go with line numbers
find . -name "kvo_test.go" -exec cat -n {} \;Repository: cinar/indicator
Length of output: 1903
Use standard test helpers and fix channel consumption to detect length mismatches.
This test bypasses the project's standard testing pattern used in all other volume indicators. It lacks deterministic assertions and uses manual checks instead of helper.CheckEquals. Additionally, consuming signalResult inside range kvoResult will hang if the channels diverge instead of failing with a clear error.
✅ Suggested fix
Replace the loop at lines 37-40 with explicit channel closing checks:
- for v := range kvoResult {
- kvoSlice = append(kvoSlice, v)
- signalSlice = append(signalSlice, <-signalResult)
- }
+ for {
+ kvoValue, kvoOk := <-kvoResult
+ signalValue, signalOk := <-signalResult
+
+ if kvoOk != signalOk {
+ t.Fatalf("kvo/signal length mismatch")
+ }
+ if !kvoOk {
+ break
+ }
+
+ kvoSlice = append(kvoSlice, kvoValue)
+ signalSlice = append(signalSlice, signalValue)
+ }Then add proper assertions per guidelines: use helper.RoundDigits() before comparing float64 values, and use helper.CheckEquals() for equality checks instead of manual validation.
Per coding guidelines: Use helper.CheckEquals for assertions in tests and Use helper.RoundDigits for floating-point number comparison in tests.
📝 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.
| for v := range kvoResult { | |
| kvoSlice = append(kvoSlice, v) | |
| signalSlice = append(signalSlice, <-signalResult) | |
| } | |
| if len(kvoSlice) == 0 { | |
| t.Fatal("KVO produced no output") | |
| } | |
| for i, v := range kvoSlice { | |
| if math.IsNaN(v) || math.IsInf(v, 0) { | |
| t.Fatalf("KVO at index %d is NaN or Inf: %v", i, v) | |
| } | |
| } | |
| t.Logf("KVO: %d values, Signal: %d values", len(kvoSlice), len(signalSlice)) | |
| } | |
| for { | |
| kvoValue, kvoOk := <-kvoResult | |
| signalValue, signalOk := <-signalResult | |
| if kvoOk != signalOk { | |
| t.Fatalf("kvo/signal length mismatch") | |
| } | |
| if !kvoOk { | |
| break | |
| } | |
| kvoSlice = append(kvoSlice, kvoValue) | |
| signalSlice = append(signalSlice, signalValue) | |
| } | |
| if len(kvoSlice) == 0 { | |
| t.Fatal("KVO produced no output") | |
| } | |
| for i, v := range kvoSlice { | |
| if math.IsNaN(v) || math.IsInf(v, 0) { | |
| t.Fatalf("KVO at index %d is NaN or Inf: %v", i, v) | |
| } | |
| } | |
| t.Logf("KVO: %d values, Signal: %d values", len(kvoSlice), len(signalSlice)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@volume/kvo_test.go` around lines 37 - 53, The current test reads from
kvoResult and signalResult together (looping over kvoResult and consuming
signalResult inside), which can hang on mismatched channel lengths and uses
manual checks; change to drain each channel independently (e.g., range kvoResult
to append to kvoSlice until closed, then range signalResult to append to
signalSlice) so length mismatches fail cleanly, then replace manual NaN/Inf and
length checks with the project test helpers: use helper.RoundDigits(...) when
comparing float64 values and assert equality with helper.CheckEquals(...) for
expected counts/values; update the assertions around kvoSlice and signalSlice
and any float comparisons to call helper.RoundDigits before passing values to
helper.CheckEquals.
| ``` | ||
| Trend = +1 if High > High[1] and Low >= Low[1] | ||
| Trend = -1 if High <= High[1] and Low < Low[1] | ||
| Trend = 0 otherwise | ||
| VF = Volume * Trend | ||
| KVO = EMA(VF, shortPeriod) - EMA(VF, longPeriod) | ||
| Signal = EMA(KVO, signalPeriod) | ||
| ``` | ||
|
|
||
| Example: | ||
|
|
||
| ``` | ||
| kvo := volume.NewKvo[float64]() | ||
| kvoResult, signalResult := kvo.Compute(highs, lows, volumes) | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to the new KVO fenced blocks.
The added KVO formula/example fences are unlabeled and trigger markdownlint MD040.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@volume/README.md` around lines 395 - 409, The fenced code blocks for the KVO
formula and the Go example are missing language identifiers; update the formula
block to use a neutral identifier (e.g., ```text) and the example block to use
the Go identifier (```go) so markdownlint MD040 is satisfied; specifically edit
the README's KVO section containing the formula block and the snippet that
creates NewKvo[float64]() and calls Compute(highs, lows, volumes) to add the
appropriate fenced-language tags.
2 participants
Describe Request
Klinger Volume Oscillator (KVO) is added.
Fixed #309
Change Type
New indicator.
Summary by CodeRabbit
New Features
Improvements
Documentation
Chores