Feature/fix aroon #276
Feature/fix aroon #276
Conversation
WalkthroughAdds generic sliding-window utilities and derived helpers: Window and SlicesReverse, Highest/Lowest, MaxSince/MinSince, unit tests for these helpers, and updates Aroon to use MaxSince/MinSince for "since last high/low" calculations. Changes
Sequence Diagram(s)sequenceDiagram
participant In as Input stream
participant Win as helper.Window
participant Ext as helper.Highest/Lowest
participant Since as helper.MaxSince/MinSince
participant Aroon as trend.Aroon
participant Out as Aroon Up/Down
In->>Win: stream elements
Win->>Ext: apply per-window extrema func
Ext-->>Win: window extrema stream
Win->>Since: compute per-window "since" counts
Since-->>Aroon: emit days-since-last-extrema
Aroon-->>Out: compute and emit Aroon Up/Down
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. ✨ 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 (
|
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
helper/slices_reverse_test.go (3)
16-18: Improve failure messages to aid debuggingInclude expected vs actual in the failure for quicker triage.
- if !slices.Equal(actual, expected) { - t.Fatal("not equal") - } + if !slices.Equal(actual, expected) { + t.Fatalf("not equal: expected=%v actual=%v", expected, actual) + }
29-31: Likewise here: print expected vs actual on failureMatching the first test improves consistency and debuggability.
- if !slices.Equal(actual, expected) { - t.Fatal("not equal") - } + if !slices.Equal(actual, expected) { + t.Fatalf("not equal: expected=%v actual=%v", expected, actual) + }
8-19: Add coverage for early termination and pivot edge casesCurrent tests don’t verify callback early-stop or boundary pivots (0/len). Adding these catches regressions in traversal order and stopping logic.
Add these tests:
func TestSlicesReverseEarlyStop(t *testing.T) { input := []int{1,2,3,4,5} // Expect only last two elements when stopping early expected := []int{5,4} actual := make([]int, 0, len(expected)) count := 0 SlicesReverse(input, 0, func(i int) bool { actual = append(actual, i) count++ return count < 2 // stop after two elements }) if !slices.Equal(actual, expected) { t.Fatalf("not equal: expected=%v actual=%v", expected, actual) } } func TestSlicesReversePivotAtLen(t *testing.T) { input := []int{1,2,3,4} expected := []int{4,3,2,1} // pivot=len behaves like pivot=0 actual := make([]int, 0, len(input)) SlicesReverse(input, len(input), func(i int) bool { actual = append(actual, i) return true }) if !slices.Equal(actual, expected) { t.Fatalf("not equal: expected=%v actual=%v", expected, actual) } }Also applies to: 21-32
helper/highest.go (1)
12-14: Mark unused callback parameter to clarify intentUse underscore for the unused index parameter to avoid linter warnings and improve readability.
-return Window(c, func(s []T, i int) T { - return slices.Max(s) +return Window(c, func(s []T, _ int) T { + return slices.Max(s) }, w)helper/window_test.go (1)
7-20: Optional: use t.Run and descriptive messages for readabilityTable-driven subtests with descriptive names ease maintenance and failure triage. Current checks rely on CheckEquals error text; adding context can help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
trend/testdata/aroon.csvis excluded by!**/*.csv
📒 Files selected for processing (13)
helper/Lowest.go(1 hunks)helper/highest.go(1 hunks)helper/highest_test.go(1 hunks)helper/lowest_test.go(1 hunks)helper/max_since.go(1 hunks)helper/max_since_test.go(1 hunks)helper/min_since.go(1 hunks)helper/min_since_test.go(1 hunks)helper/slices_reverse.go(1 hunks)helper/slices_reverse_test.go(1 hunks)helper/window.go(1 hunks)helper/window_test.go(1 hunks)trend/aroon.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
helper/max_since_test.go (3)
helper/max_since.go (1)
MaxSince(14-31)helper/bst_test.go (2)
TestMinAndMax(59-79)TestEmptyMax(90-97)helper/sync_test.go (1)
TestSync(13-33)
helper/window.go (2)
helper/shift.go (1)
Shift(15-27)helper/skip.go (1)
Skip(15-30)
🔇 Additional comments (7)
helper/window.go (1)
20-30: The sliding window logic is correctly implemented.The circular buffer approach efficiently manages memory while maintaining the sliding window. The offset parameter correctly indicates the start of the current window within the circular buffer, and the initial phase properly handles partial windows.
helper/max_since_test.go (1)
7-16: LGTM! Test data and logic are correct.The test correctly validates the
MaxSincefunction with appropriate input data and expected outputs. The expected values match the correct "since last maximum" calculations for a sliding window of size 3.helper/min_since_test.go (1)
5-14: LGTM! Test data and logic are correct.The test correctly validates the
MinSincefunction with appropriate input data and expected outputs. The expected values match the correct "since last minimum" calculations for a sliding window of size 3.helper/lowest_test.go (1)
11-21: LGTM! Test data and logic are correct.The test correctly validates the
Lowestfunction with appropriate input data and expected outputs. The expected values match the correct minimum calculations for a sliding window of size 3.helper/Lowest.go (1)
11-15: LGTM! Clean and efficient implementation.The
Lowestfunction correctly leverages the genericWindowfunction andslices.Minto compute sliding window minimums. The implementation is concise and follows the established pattern.trend/aroon.go (1)
49-50: Excellent fix for the Aroon indicator calculation issue.This change directly addresses issue #124 by replacing the generic
helper.Sincefunction with specializedhelper.MaxSinceandhelper.MinSincefunctions. The new functions correctly calculate the number of days since the last maximum/minimum within the specified period, rather than when the value last changed. This ensures accurate Aroon Up and Aroon Down calculations.helper/highest_test.go (1)
11-21: LGTM! Test data and logic are correct.The test correctly validates the
Highestfunction with appropriate input data and expected outputs. The expected values match the correct maximum calculations for a sliding window of size 3.
| return Window(c, func(w []T, i int) T { | ||
| since := 0 | ||
| found := false | ||
| m := slices.Max(w) | ||
| SlicesReverse(w, i, func(n T) bool { | ||
| if found && n < m { | ||
| return false | ||
| } | ||
| since++ | ||
| if n == m { | ||
| found = true | ||
| } | ||
| return true | ||
| }) | ||
| return T(since - 1) | ||
| }, w) |
There was a problem hiding this comment.
Correctness: off-by-run when max repeats at the end; returns wrong “since last high”
Current logic counts the entire trailing run of maxima and subtracts 1, which breaks Aroon when the maximum repeats at the window end. Example window [3, 5, 5] should yield 0 (max occurred now), but this returns 1.
Fix: count steps until the first encounter of the maximum from the end, then stop.
func MaxSince[T Number](c <-chan T, w int) <-chan T {
- return Window(c, func(w []T, i int) T {
- since := 0
- found := false
- m := slices.Max(w)
- SlicesReverse(w, i, func(n T) bool {
- if found && n < m {
- return false
- }
- since++
- if n == m {
- found = true
- }
- return true
- })
- return T(since - 1)
- }, w)
+ return Window(c, func(win []T, i int) T {
+ m := slices.Max(win)
+ steps := 0
+ SlicesReverse(win, i, func(n T) bool {
+ if n == m {
+ return false // stop at the most recent occurrence of max
+ }
+ steps++
+ return true
+ })
+ return T(steps)
+ }, w)
}Consider adding unit tests covering:
- Repeated max at the end: expect 0.
- Max one step back: expect 1.
- Max at farthest position in the window: expect w-1.
🤖 Prompt for AI Agents
In helper/max_since.go around lines 15 to 30, the current logic incorrectly
counts the entire trailing run of maximum values and subtracts one, causing an
off-by-one error when the maximum repeats at the end of the window. To fix this,
modify the counting logic to increment steps only until the first encounter of
the maximum from the end, then stop counting immediately. Additionally, add unit
tests to cover cases where the max repeats at the end (expect 0), max is one
step back (expect 1), and max is at the farthest position in the window (expect
w-1).
| return Window(c, func(w []T, i int) T { | ||
| since := 0 | ||
| found := false | ||
| m := slices.Min(w) | ||
| SlicesReverse(w, i, func(n T) bool { | ||
| if found && n > m { | ||
| return false | ||
| } | ||
| since++ | ||
| if n == m { | ||
| found = true | ||
| } | ||
| return true | ||
| }) | ||
| return T(since - 1) | ||
| }, w) |
There was a problem hiding this comment.
Correctness: off-by-run when min repeats at the end; returns wrong “since last low”
Same issue as MaxSince. For window [1, 1, 3], the correct “since last low” is 0 (min occurred now), but current code returns 1.
Fix by stopping at the first encounter of the minimum from the end.
func MinSince[T Number](c <-chan T, w int) <-chan T {
- return Window(c, func(w []T, i int) T {
- since := 0
- found := false
- m := slices.Min(w)
- SlicesReverse(w, i, func(n T) bool {
- if found && n > m {
- return false
- }
- since++
- if n == m {
- found = true
- }
- return true
- })
- return T(since - 1)
- }, w)
+ return Window(c, func(win []T, i int) T {
+ m := slices.Min(win)
+ steps := 0
+ SlicesReverse(win, i, func(n T) bool {
+ if n == m {
+ return false // stop at the most recent occurrence of min
+ }
+ steps++
+ return true
+ })
+ return T(steps)
+ }, w)
}Please add complementary tests mirroring the MaxSince cases for repeated minima.
📝 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.
| return Window(c, func(w []T, i int) T { | |
| since := 0 | |
| found := false | |
| m := slices.Min(w) | |
| SlicesReverse(w, i, func(n T) bool { | |
| if found && n > m { | |
| return false | |
| } | |
| since++ | |
| if n == m { | |
| found = true | |
| } | |
| return true | |
| }) | |
| return T(since - 1) | |
| }, w) | |
| func MinSince[T Number](c <-chan T, w int) <-chan T { | |
| return Window(c, func(win []T, i int) T { | |
| m := slices.Min(win) | |
| steps := 0 | |
| SlicesReverse(win, i, func(n T) bool { | |
| if n == m { | |
| return false // stop at the most recent occurrence of min | |
| } | |
| steps++ | |
| return true | |
| }) | |
| return T(steps) | |
| }, w) | |
| } |
🤖 Prompt for AI Agents
In helper/min_since.go around lines 14 to 29, the function incorrectly
calculates the "since last low" value when the minimum value repeats at the end
of the window, causing an off-by-one error. To fix this, modify the loop to stop
immediately upon encountering the first minimum value from the end, ensuring the
count reflects the correct distance. Additionally, add test cases similar to
those for MaxSince that cover scenarios with repeated minimum values to verify
correctness.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
trend/aroon_test.go (1)
49-54: Consider reusing the existing AroonData struct.The test defines a local
AroonDatastruct that's identical to the one in the existingTestAroonfunction (lines 15-20). This creates code duplication.Consider moving the
AroonDatastruct to package level or a test helper to avoid duplication:+type AroonData struct { + High float64 + Low float64 + Up float64 + Down float64 +} + func TestAroon(t *testing.T) { - type AroonData struct { - High float64 - Low float64 - Up float64 - Down float64 - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
trend/aroon_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
trend/aroon_test.go (2)
trend/aroon.go (1)
NewAroon(37-41)helper/skip.go (1)
Skip(15-30)
🔇 Additional comments (5)
trend/aroon_test.go (5)
46-48: Good documentation with external reference.The comment provides clear context by referencing the Rust implementation being compared against, which helps with test maintainability and verification.
55-71: Test data looks comprehensive and well-structured.The test data covers the expected patterns:
- Initial "None" cases (lines 56-60) where no meaningful Aroon values can be calculated
- Transition period with valid Up/Down calculations
- Good coverage of different Aroon value ranges (0, 20, 40, 60, 80, 100)
The data structure matches the referenced Rust implementation, ensuring accurate cross-validation.
73-84: Test setup follows established patterns correctly.The test properly:
- Uses
helper.SliceToChanto convert test data to channels- Creates a new Aroon instance with default configuration
- Uses
helper.Duplicateto create multiple input streams- Maps the channels to extract high/low values
- Skips the initial period-1 values for expected results alignment
This matches the pattern used in the existing
TestAroonfunction.
85-91: Test execution and validation implemented correctly.The test properly:
- Calls the
aroon.Computemethod with high and low channels- Uses
helper.CheckEqualsto compare actual vs expected results for both Up and Down channels- Handles errors appropriately with
t.FatalThis follows the same validation pattern as the existing test.
46-91: Verified Test Data Matches Rust ReferenceThe hard-coded expected values in TestAroonCompareRust have been confirmed to exactly match the assertions in sabinchitrakar/aroon-rs’s
docs/aroon.md. No updates needed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
helper/window.go (1)
15-18: Input validation properly addresses the previous concern.The addition of window size validation correctly handles the edge case identified in the past review comment. Returning a closed channel for invalid window sizes is a clean approach that prevents panics and provides predictable behavior.
🧹 Nitpick comments (1)
helper/max_since_test.go (1)
7-27: Consider additional test coverage for robustness.While the current tests effectively validate the core MaxSince functionality, consider adding tests for:
- Different window sizes (1, 2, larger values)
- Edge cases like empty input or single elements
- Window size larger than input sequence
This would enhance robustness, though the existing coverage is adequate for the primary use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
helper/max_since_test.go(1 hunks)helper/min_since_test.go(1 hunks)helper/slices_reverse.go(1 hunks)helper/window.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- helper/min_since_test.go
- helper/slices_reverse.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
helper/max_since_test.go (1)
helper/max_since.go (1)
MaxSince(14-31)
🔇 Additional comments (5)
helper/window.go (3)
20-36: Solid circular buffer implementation with correct offset calculation.The sliding window implementation correctly handles both the initial filling phase and the full window phase. The offset calculation
(n+1)%wproperly indicates the logical start of the window within the circular buffer, enabling the callback function to reconstruct the correct element order when needed.
20-21: Excellent goroutine lifecycle management.The use of
defer close(r)ensures proper cleanup, and the channel reading pattern correctly detects input channel closure for clean termination. This follows Go best practices and prevents goroutine leaks.Also applies to: 25-26
7-11: Clear and helpful documentation.The function documentation effectively explains the sliding window behavior and includes the important note about slice ordering and offset usage, which is crucial for users to understand the circular buffer implementation.
helper/max_since_test.go (2)
7-16: Well-designed test cases with correct expected values.Both test functions cover important scenarios:
TestMaxSincetests a comprehensive sequence with various patterns (increasing, decreasing, duplicates)TestMaxSinceAtEndspecifically validates the edge case where multiple equal maximums appear consecutivelyThe expected results correctly reflect the MaxSince algorithm behavior, which counts positions back to the first occurrence of the maximum within each sliding window.
Also applies to: 18-27
8-15: Clean testing methodology using appropriate helper functions.The tests properly utilize
SliceToChanandCheckEqualshelper functions to work with the channel-based API. The use oft.Fatalprovides clear failure reporting, and the testing pattern is consistent and follows Go best practices.Also applies to: 19-26
hwde
force-pushed
the
feature/fix-aroon
branch
from
3ab1515 to
92c40ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
helper/max_since.go (1)
15-30: Fix off-by-run: duplicates of the maximum at the end should yield 0, not 1Current logic counts the entire trailing run of maxima and subtracts 1. This breaks Aroon when the max repeats at the window end (e.g., [3,5,5] yields 1 instead of 0).
-func MaxSince[T Number](c <-chan T, w int) <-chan T { - return Window(c, func(w []T, i int) T { - since := 0 - found := false - m := slices.Max(w) - SlicesReverse(w, i, func(n T) bool { - if found && n < m { - return false - } - since++ - if n == m { - found = true - } - return true - }) - return T(since - 1) - }, w) -} +func MaxSince[T Number](c <-chan T, w int) <-chan T { + return Window(c, func(win []T, i int) T { + m := slices.Max(win) + steps := 0 + SlicesReverse(win, i, func(n T) bool { + if n == m { + return false // stop at the most recent occurrence of max + } + steps++ + return true + }) + return T(steps) + }, w) +}Consider adding unit tests covering:
- Repeated max at the end: expect 0.
- Max one step back: expect 1.
- Max at farthest position (full window): expect w-1.
🧹 Nitpick comments (5)
helper/window.go (3)
7-11: Clarify and tighten the Window doc commentThe current phrasing is grammatically off and ambiguous about the meaning of index i. Recommend rewording for clarity.
-// Window returns a channel that emits the passed function result -// within a sliding window of size w from the input channel c. -// Note: the slice is in the same order than in source channel -// but the 1st element may not be 0, use modulo window size if -// order is important. +// Window returns a channel that emits f applied to a sliding window of size w over c. +// For the initial (partial) windows, f receives a slice in source order and i=0. +// Once the window is full, f receives the circular buffer backing array and i points +// to the logical start (use modulo indexing with i for order-sensitive logic).
13-13: Buffer the output channel to match input channel capacity to reduce blockingOther helpers (e.g., Shift, Skip) preserve capacity using cap(c). Doing the same here avoids unnecessary blocking and aligns with existing patterns.
- r := make(chan T) + r := make(chan T, cap(c))
25-35: Simplify the loop; avoid incrementing n after input channel closesMore idiomatic and avoids the extra n increment on a closed channel path.
- for ok := true; ok; { - if h[n], ok = <-c; ok { - if cnt < w { - cnt++ - r <- f(h[:cnt], 0) - } else { - r <- f(h, (n+1)%w) - } - } - n = (n + 1) % w - } + for v := range c { + h[n] = v + if cnt < w { + cnt++ + r <- f(h[:cnt], 0) + } else { + r <- f(h, (n+1)%w) + } + n = (n + 1) % w + }helper/max_since.go (1)
11-13: Clarify MaxSince semantics in the doc commentMake it explicit we count since the most recent maximum (Aroon-compatible).
-// MaxSince returns a channel of T indicating since when -// (number of previous values) the respective value was the maximum -// within the window of size w. +// MaxSince returns a channel with the number of periods since the most recent +// maximum within the last w observations (0 if the current observation is the max).helper/window_test.go (1)
1-64: Add tests for invalid window sizes and for windows larger than the inputEdge cases strengthen confidence and prevent regressions for w<=0 and w>len(input).
@@ func TestWindowIndexElement(t *testing.T) { @@ } + +func TestWindowInvalidSizes(t *testing.T) { + input := SliceToChan([]int{1, 2, 3}) + // w = 0 + out0 := Window(input, func(v []int, i int) int { return 42 }, 0) + if err := CheckEquals(out0, SliceToChan([]int{})); err != nil { + t.Fatal(err) + } + // w < 0 + outNeg := Window(SliceToChan([]int{1, 2, 3}), func(v []int, i int) int { return 42 }, -1) + if err := CheckEquals(outNeg, SliceToChan([]int{})); err != nil { + t.Fatal(err) + } +} + +func TestWindowLargerThanInput(t *testing.T) { + input := SliceToChan([]int{1, 2}) + // With w > len(input), only partial windows exist. + expected := SliceToChan([]int{1, 3}) + out := Window(input, func(v []int, i int) int { + sum := 0 + for _, n := range v { + sum += n + } + return sum + }, 5) + if err := CheckEquals(out, expected); err != nil { + t.Fatal(err) + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
trend/testdata/aroon.csvis excluded by!**/*.csv
📒 Files selected for processing (14)
helper/Lowest.go(1 hunks)helper/highest.go(1 hunks)helper/highest_test.go(1 hunks)helper/lowest_test.go(1 hunks)helper/max_since.go(1 hunks)helper/max_since_test.go(1 hunks)helper/min_since.go(1 hunks)helper/min_since_test.go(1 hunks)helper/slices_reverse.go(1 hunks)helper/slices_reverse_test.go(1 hunks)helper/window.go(1 hunks)helper/window_test.go(1 hunks)trend/aroon.go(1 hunks)trend/aroon_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- trend/aroon.go
- helper/highest_test.go
- helper/Lowest.go
- helper/lowest_test.go
- helper/max_since_test.go
- helper/highest.go
- trend/aroon_test.go
- helper/slices_reverse.go
- helper/slices_reverse_test.go
- helper/min_since.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
helper/min_since_test.go (1)
helper/min_since.go (1)
MinSince(13-30)
helper/window_test.go (1)
helper/window.go (1)
Window(12-39)
helper/max_since.go (5)
helper/window.go (1)
Window(12-39)helper/slices_reverse.go (1)
SlicesReverse(6-21)helper/sync.go (1)
CommonPeriod(24-26)helper/shift.go (1)
Shift(15-27)helper/skip.go (1)
Skip(15-30)
🔇 Additional comments (5)
helper/min_since_test.go (2)
16-25: Ignore incorrect expectation update for trailing minimaThe current MinSince implementation scans the entire window when the minimum value repeats at the end, so it returns windowSize–1 for a run of equal minima. For input
[48, 50, 50, 50, 49, 49, 49]with w=3, you get:• Window 1–3 ([48], [48,50], [48,50,50]) → 0, 1, 2
• Window 2–4 ([50,50,50]) → 2 (since all three are equal minima)
• Windows ending in 49 ([50,50,49], [50,49,49], [49,49,49]) → 0, 1, 2The existing expected output
SliceToChan([]int{0, 1, 2, 2, 0, 1, 2})matches this behavior, so no test change is needed.
Likely an incorrect or invalid review comment.
27-36: No changes needed for MinSinceFromStart testThe existing test and implementation are correct and consistent with the intended semantics:
– MinSince counts how many values have passed since the last time the window’s minimum occurred, including repeated minima.
– In the partial window [1, 1], the second “1” indeed has its previous minimum one step back, yielding 1.
– Changing the expectation to [0, 0, 1] or altering the scanning logic would break the other MinSince tests (e.g., repeated‐min sequences in TestMinSince and TestMinSinceAtEnd).Please disregard the proposed diff and implementation change.
Likely an incorrect or invalid review comment.
helper/window_test.go (3)
7-20: LGTM: Validates 1-sized sliding windows correctlyGood coverage of the partial-window behavior and using i=0 for initial windows.
22-39: LGTM: Confirms aggregation over partial and full windowsSum behavior across window growth and steady-state is exercised well.
41-63: LGTM: Index semantics of the circular buffer are well testedThe i parameter and element selection are validated thoroughly.
| func TestMinSince(t *testing.T) { | ||
| input := SliceToChan([]int{48, 50, 50, 50, 49, 49, 51}) | ||
| expected := SliceToChan([]int{0, 1, 2, 2, 0, 1, 2}) | ||
| actual := MinSince(input, 3) | ||
|
|
||
| err := CheckEquals(actual, expected) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tests encode “first occurrence of min” (w-1 when all equal), not “most recent min” required by Aroon
Per issue #124 and Aroon’s definition, “since” should be counted from the most recent minimum within the window (0 if current point is the min). The current expected values reflect counting from the earliest occurrence. Update expectations accordingly.
- expected := SliceToChan([]int{0, 1, 2, 2, 0, 1, 2})
+ // Most recent-min semantics: count steps back to the latest min in the window.
+ expected := SliceToChan([]int{0, 1, 2, 0, 0, 0, 1})If desired, I can open a follow-up to adjust helper/MinSince to match these semantics (mirroring the MaxSince fix) and extend tests.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the MinSince implementation
rg -n -C5 "func MinSince" --color=neverLength of output: 625
🏁 Script executed:
#!/bin/bash
set -e
# Show the full Window function
rg -n -C20 "func Window" --color=never
# Show the full SlicesReverse implementation
rg -n -C20 "func SlicesReverse" --color=neverLength of output: 2548
Update MinSince test to use most-recent-min semantics
The TestMinSince currently counts from the first window minimum (yielding 2 when all values are equal). Per Aroon’s definition, “since” must reset to 0 whenever the current value is the window minimum and otherwise count bars since the most recent minimum.
• helper/min_since_test.go: in TestMinSince, update the expected channel to reflect most-recent‐min semantics.
- expected := SliceToChan([]int{0, 1, 2, 2, 0, 1, 2})
+ // most-recent-min semantics: number of bars since the latest window minimum (0 if current is the min)
+ expected := SliceToChan([]int{0, 1, 2, 0, 0, 0, 1})You may also open a follow-up to adjust helper/MinSince (mirroring the MaxSince fix) and extend tests accordingly.
📝 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 TestMinSince(t *testing.T) { | |
| input := SliceToChan([]int{48, 50, 50, 50, 49, 49, 51}) | |
| expected := SliceToChan([]int{0, 1, 2, 2, 0, 1, 2}) | |
| actual := MinSince(input, 3) | |
| err := CheckEquals(actual, expected) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| func TestMinSince(t *testing.T) { | |
| input := SliceToChan([]int{48, 50, 50, 50, 49, 49, 51}) | |
| // most-recent-min semantics: number of bars since the latest window minimum (0 if current is the min) | |
| expected := SliceToChan([]int{0, 1, 2, 0, 0, 0, 1}) | |
| actual := MinSince(input, 3) | |
| err := CheckEquals(actual, expected) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In helper/min_since_test.go around lines 5 to 14, the TestMinSince expected
values use first-window-min semantics; update the expected channel to follow
most-recent-min semantics so results are [0,1,2,0,0,0,1] for input
[]int{48,50,50,50,49,49,51} with window 3 (i.e., reset to 0 when the current
value is the window minimum and otherwise count bars since the most recent
minimum).
There was a problem hiding this comment.
It is hard to find really an exact definition how to calculate MinSince and MaxSince. A explanation of on old R implementation (https://github.com/sabinchitrakar/aroon-rs/blob/master/docs/aroon.md) notes:
Aroon up (down) is the elapsed time, expressed as a percentage, between today
and the highest (lowest) price in the last \code{n} periods. If today's
price is a new high (low) Aroon up (down) will be 100. Each subsequent period
without another new high (low) causes Aroon up (down) to decrease by (1 /
\code{n}) x 100.
So another test function of Aroon() might follow that:
func TestAroonValidateDefinition(t *testing.T) {
// compares to: https://github.com/sabinchitrakar/aroon-rs/blob/master/docs/aroon.md
type AroonData struct {
High float64
Low float64
Up float64
Down float64
}
var testData = []AroonData{
{High: 11, Low: 9, Up: 0, Down: 0}, // None
{High: 10, Low: 9, Up: 0, Down: 0}, // None
{High: 9, Low: 9, Up: 100, Down: 100}, // None
{High: 9, Low: 9, Up: 67, Down: 67}, // None
{High: 9, Low: 9, Up: 33, Down: 33}, // None
}
input := helper.SliceToChan[AroonData](testData)
aroon := &trend.Aroon[float64]{
Period: 3,
}
inputs := helper.Duplicate(input, 4)
high := helper.Map(inputs[0], func(row AroonData) float64 { return row.High })
low := helper.Map(inputs[1], func(row AroonData) float64 { return row.Low })
expectedUp := helper.Map(inputs[2], func(row AroonData) float64 { return row.Up })
expectedDown := helper.Map(inputs[3], func(row AroonData) float64 { return row.Down })
expectedUp = helper.Skip(expectedUp, aroon.Period-1)
expectedDown = helper.Skip(expectedDown, aroon.Period-1)
actualUp, actualDown := aroon.Compute(high, low)
err := helper.CheckEquals(actualUp, expectedUp, actualDown, expectedDown)
if err != nil {
t.Fatal(err)
}
}
There was a problem hiding this comment.
🧩 Analysis chain
@hwde Thank you for that excellent clarification! The R implementation reference you found provides exactly the definition we needed to resolve the MinSince/MaxSince semantics question.
Your quote is particularly illuminating:
"If today's price is a new high (low) Aroon up (down) will be 100. Each subsequent period without another new high (low) causes Aroon up (down) to decrease by (1/n) x 100."
This definitively confirms that Aroon should use "most recent" semantics - when the current value is the window high/low, it should return 0 periods since (resulting in 100% Aroon value), and otherwise count from the most recent occurrence.
Your proposed TestAroonValidateDefinition test case would be an excellent addition to validate the implementation against this authoritative definition. It provides concrete expected values that can serve as a reference benchmark.
🏁 Script executed:
#!/bin/bash
# Let's verify if the current implementation already handles this correctly by checking the existing Aroon test
cat trend/aroon_test.go
Length of output: 3098
Perfect! I can see that there's already a TestAroonCompareRust function that validates against the exact same Rust implementation reference you mentioned. This is excellent - it shows the current implementation is already being tested against that authoritative source.
Looking at the existing test data in TestAroonCompareRust, I can see the correct behavior patterns:
- When a new high occurs (like 88.00 in the data), AroonUp jumps to 100
- When a new low occurs, AroonDown becomes 100
- Values properly decrease over time when no new extremes are found
Your proposed TestAroonValidateDefinition would still be a valuable addition because:
- It uses simpler, more predictable data (repeated 9s) making it easier to manually verify
- It serves as an educational example of the core Aroon logic
- It provides additional test coverage with different edge cases
The fact that TestAroonCompareRust is already passing suggests that the current MaxSince/MinSince implementation is likely working correctly with the "most recent" semantics. Your test case would be a nice complement to validate this behavior with clearer, more obvious data patterns.
Would you like to add your TestAroonValidateDefinition test case to provide additional validation coverage?
✏️ Learnings added
Learnt from: hwde
PR: cinar/indicator#276
File: helper/min_since_test.go:5-14
Timestamp: 2025-08-12T05:34:50.485Z
Learning: The Aroon indicator measures elapsed time as a percentage between today and the highest/lowest price in the last n periods. When today's price is a new high/low, Aroon should be 100. Each subsequent period without another new high/low causes Aroon to decrease by (1/n) x 100. This confirms that MinSince/MaxSince should use "most recent" semantics, returning 0 when current value is the window extremum.
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.
hwde
force-pushed
the
feature/fix-aroon
branch
from
87195f1 to
3c11e79
Compare
add highest and lowest add window, use it from highest and lowest add Max/MinSince with helper Window and SlicesReverse change aroon indicator as suggested by cinar#124 add a another test for Aroon applied some ai suggesttions update aroon strategy test data to match new actions add another Aroon Test inspired by an R implementation Update testdata to fix depending test of no_loss_strategy fix depending testdata
hwde
force-pushed
the
feature/fix-aroon
branch
from
50d7785 to
705d549
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #276 +/- ##
==========================================
+ Coverage 91.66% 91.73% +0.06%
==========================================
Files 181 188 +7
Lines 6540 6643 +103
==========================================
+ Hits 5995 6094 +99
- Misses 484 487 +3
- Partials 61 62 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
helper/slices_reverse.go (1)
8-10: Bounds checks addressed — good defensive programmingEarly return on empty slice and out-of-range i prevents panics and undefined traversal.
🧹 Nitpick comments (7)
helper/highest_test.go (2)
12-15: Broaden coverage with table-driven tests (multiple windows, edge patterns).Add more cases to harden the contract: window=1, window>len(input), strictly increasing/decreasing sequences, negatives, and duplicate plateaus. This will guard against regressions in Window/Highest.
Here’s a compact table-driven test you can drop alongside the current one:
func TestHighest_Table(t *testing.T) { t.Parallel() type tc struct { name string in []int w int expect []int } for _, c := range []tc{ { name: "w=1 returns input", in: []int{3, 2, 5, 1}, w: 1, expect: []int{3, 2, 5, 1}, }, { name: "w>len(input) behaves like growing window", in: []int{1, 4, 2}, w: 10, expect: []int{1, 4, 4}, }, { name: "strictly increasing sequence", in: []int{1, 2, 3, 4}, w: 3, expect: []int{1, 2, 3, 4}, }, { name: "strictly decreasing sequence", in: []int{9, 7, 5, 3}, w: 3, expect: []int{9, 9, 9, 7}, }, { name: "duplicates within window", in: []int{2, 5, 5, 1, 5}, w: 3, expect: []int{2, 5, 5, 5, 5}, }, { name: "negative values", in: []int{-5, -2, -3, -1}, w: 2, expect: []int{-5, -2, -2, -1}, }, } { t.Run(c.name, func(t *testing.T) { inCh := SliceToChan(c.in) wantCh := SliceToChan(c.expect) if err := CheckEquals(Highest(inCh, c.w), wantCh); err != nil { t.Fatal(err) } }) } }
17-20: Nit: inline the error check for brevity.Minor readability tweak; avoid the extra var.
- err := CheckEquals(actual, expected) - if err != nil { - t.Fatal(err) - } + if err := CheckEquals(actual, expected); err != nil { + t.Fatal(err) + }helper/Lowest.go (2)
9-14: Clarify docstring about edge cases (w <= 0, warm-up period)Document that Window closes immediately when w <= 0 and that during warm-up (before w items), the minimum of seen items is emitted. This helps users reason about initial outputs.
Apply:
-// Lowest returns a channel that emits the lowest value -// within a sliding window of size w from the input channel c. +// Lowest returns a channel that emits the lowest value within a sliding window of size w +// from the input channel c. During warm-up (before w items are received), it emits the +// minimum of the items seen so far. If w <= 0, the returned channel is closed immediately.
1-6: File naming consistencyGo files are typically lowercase. Consider renaming Lowest.go to lowest.go to align with common Go conventions and the rest of the helper package.
helper/max_since_test.go (1)
1-27: Broaden test coverage and edge casesConsider adding:
- A case where w = 1 (always 0).
- A case where w > len(series) (warm-up behavior).
- A case where the last value equals but does not exceed the prior max (e.g., [1, 3, 3] with w=3 → 0).
- Negative/zero window sizes return closed channel (no emissions) to match Window behavior.
A table-driven test will keep these concise.
helper/slices_reverse.go (2)
3-6: Clarify the semantics of index i in the docThis function iterates the slice in reverse circular order starting from i-1, then wraps: i-1 … 0, then len(r)-1 … i. Explicitly documenting the “circular wrap” and that each element is visited at most once will make callers (e.g., Window users) less error-prone.
Apply:
-// SlicesReverse loops through a slice in reverse order starting from the given index. -// The given function is called for each element in the slice. If the function returns false, -// the loop is terminated. +// SlicesReverse traverses r in reverse circular order relative to index i: +// it visits i-1 down to 0, then wraps and visits len(r)-1 down to i. For each element, f is +// called; if f returns false, traversal stops. Each element is visited at most once.
11-20: Consider exposing the element index to the callbackSome callers may need the logical or physical index during traversal. A variant like SlicesReverseIndex[T any](r []T, i int, f func(idx int, v T) bool) would avoid capturing external counters.
📜 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 (4)
strategy/decorator/testdata/no_loss_strategy.csvis excluded by!**/*.csvstrategy/decorator/testdata/stop_loss_strategy.csvis excluded by!**/*.csvstrategy/trend/testdata/aroon_strategy.csvis excluded by!**/*.csvtrend/testdata/aroon.csvis excluded by!**/*.csv
📒 Files selected for processing (14)
helper/Lowest.go(1 hunks)helper/highest.go(1 hunks)helper/highest_test.go(1 hunks)helper/lowest_test.go(1 hunks)helper/max_since.go(1 hunks)helper/max_since_test.go(1 hunks)helper/min_since.go(1 hunks)helper/min_since_test.go(1 hunks)helper/slices_reverse.go(1 hunks)helper/slices_reverse_test.go(1 hunks)helper/window.go(1 hunks)helper/window_test.go(1 hunks)trend/aroon.go(1 hunks)trend/aroon_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- helper/min_since_test.go
- trend/aroon_test.go
- helper/highest.go
- helper/min_since.go
- helper/slices_reverse_test.go
- trend/aroon.go
- helper/lowest_test.go
- helper/window_test.go
- helper/window.go
- helper/max_since.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
helper/Lowest.go (1)
helper/window.go (1)
Window(12-39)
helper/max_since_test.go (1)
helper/max_since.go (1)
MaxSince(14-31)
helper/highest_test.go (1)
helper/highest.go (1)
Highest(11-15)
🔇 Additional comments (5)
helper/highest_test.go (3)
11-16: Test case correctly validates sliding-window maximum behavior (w=3).The expected sequence matches the semantics of a windowed max with partial warm-up windows. Good coverage of the rolling behavior and boundaries within a short series.
11-21: Confirmed: go.mod sets go 1.22 — slices.Max is supportedFound go.mod: "go 1.22". slices.Max is used in:
- helper/highest.go
- helper/max_since.go
- helper/sync.go
No changes required.
11-21: No extra tests needed — MaxSince/MinSince already validate “most recent” semantics.Existing code/tests show the requested behavior:
- helper/max_since.go / helper/min_since.go — implementation uses slices.Max/Min with SlicesReverse+found logic that selects the most recent match.
- helper/max_since_test.go — test with consecutive equal maxima (…52,52,52…) expecting [0,0,1,0,1,2,0,1,0] (w=3).
- helper/min_since_test.go — tests with duplicate minima (…49,49…) expecting [0,1,2,2,0,1,2] (w=3).
Ignore the original suggestion to add the complementary tests — the repository already covers the “most recent high/low” cases.
Likely an incorrect or invalid review comment.
helper/Lowest.go (2)
11-15: Implementation aligns with Window/Highest pattern; logic is correctUsing Window with slices.Min correctly emits the minimum for each sliding window and ignores the ring start index as expected for min/max operations. The function is safe for partial windows (cnt < w) because Window never passes an empty slice.
7-7: No action needed — repository declares Go ≥1.21Verified the go versions from go.mod files:
- /go.mod: go 1.22
- /mcp/go.mod: go 1.23
slices.Min (Go 1.21+) is supported by these toolchain versions, so the import in helper/Lowest.go is fine.
| func TestHighest(t *testing.T) { | ||
| input := SliceToChan([]int{48, 52, 50, 49, 10}) | ||
| expected := SliceToChan([]int{48, 52, 52, 52, 50}) | ||
| window := 3 | ||
| actual := Highest(input, window) | ||
|
|
||
| err := CheckEquals(actual, expected) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Define and test behavior for invalid window sizes (w <= 0).
If Highest/Window assumes w > 0, add a test that verifies a panic (or an explicit error contract). This prevents silent misconfiguration and future regressions.
Example panic test if w<=0 should not be allowed:
func TestHighest_InvalidWindowPanics(t *testing.T) {
defer func() {
if r := recover(); r == nil {
t.Fatal("expected panic for invalid window size")
}
}()
_ = Highest(SliceToChan([]int{1, 2, 3}), 0) // or -1
// Drain to ensure evaluation if needed:
// for range Highest(...) {}
}If the intended behavior is not to panic, please clarify the contract and I can adjust the test accordingly.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for Highest definitions/usages..."
rg -n --hidden --no-ignore -S "func Highest" || true
rg -n --hidden --no-ignore -S "Highest(" || true
echo
echo "Searching for helper/highest.go and related files..."
rg -n --hidden --no-ignore -S "highest" || true
echo
echo "Showing candidate Go files containing 'Highest' for inspection..."
files=$(rg -n --hidden --no-ignore -S "Highest" -l || true)
for f in $files; do
echo "----- FILE: $f -----"
sed -n '1,240p' "$f"
echo
doneLength of output: 19062
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for Window definitions/usages..."
rg -n --hidden --no-ignore -S "func Window" || true
rg -n --hidden --no-ignore -S " Window(" || true
rg -n --hidden --no-ignore -S "Window(" || true
echo
echo "Showing candidate files that define or mention Window..."
files=$(rg -n --hidden --no-ignore -S "func Window|Window(" -l || true)
for f in $files; do
echo "----- FILE: $f -----"
sed -n '1,240p' "$f"
echo
doneLength of output: 775
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "----- helper/window.go -----"
sed -n '1,240p' helper/window.go || true
echo
echo "----- Search for uses of Window (fixed string) -----"
rg -n --hidden --no-ignore -F "Window(" || true
echo
echo "----- Search for tests referencing invalid window sizes -----"
rg -n --hidden --no-ignore -S "window <= 0|w <= 0|w<=0|invalid window|invalid.*window" || trueLength of output: 1924
Window returns a closed channel for w <= 0 — test for "no output", don't expect a panic.
Window already guards w <= 0 by closing and returning the result channel (helper/window.go — the if w <= 0 { close(r); return r } block). Highest delegates to Window (helper/highest.go), so calling Highest(..., 0) or Highest(..., -1) yields a closed channel rather than panicking.
Files to update:
- helper/window.go — current behavior: close+return when w <= 0.
- helper/highest.go — uses Window, inherits that behavior.
- Tests to add/update: helper/highest_test.go (and/or helper/window_test.go).
Suggested replacement test (assert no values emitted):
func TestHighest_InvalidWindowReturnsClosedChannel(t *testing.T) {
input := SliceToChan([]int{1, 2, 3})
actual := Highest(input, 0) // also test -1 if desired
// immediate receive should indicate closed channel (no values)
if _, ok := <-actual; ok {
t.Fatal("expected closed channel (no values) for w <= 0")
}
}If you prefer a panic-based contract instead, change Window to panic on w <= 0 and update callers/tests accordingly. Otherwise, add the above test to lock in the current behavior.
🤖 Prompt for AI Agents
In helper/highest_test.go around lines 11 to 21, add a new test that asserts
Highest returns a closed channel for w <= 0 instead of panicking: create an
input channel (e.g., SliceToChan([]int{1,2,3})), call Highest(input, 0) (and
optionally Highest(input, -1)), then perform an immediate receive and fail the
test if the receive returns ok==true; keep the existing TestHighest intact. This
locks in the current Window behavior (close+return for non-positive window)
without changing Highest or Window implementation.
| func TestMaxSince(t *testing.T) { | ||
| input := SliceToChan([]int{48, 49, 47, 52, 52, 52, 53, 50, 55}) | ||
| expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 0, 1, 0}) | ||
| actual := MaxSince(input, 3) | ||
|
|
||
| err := CheckEquals(actual, expected) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Expected values reflect “earliest occurrence” of the max, not “last (most recent) high”
Per the PR objective and Aroon definition, “periods since the last high” should measure distance to the most recent occurrence of the window’s maximum. The current expected values treat ties by picking the earliest occurrence in the window, which yields non-zero distances even when the latest value equals the maximum (e.g., windows like [52, 52, 52] should produce 0).
Update expected to measure distance to the most recent max:
Please confirm the intended tie-break for duplicates (most recent vs earliest). If “most recent” is correct, apply:
- expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 0, 1, 0})
+ expected := SliceToChan([]int{0, 0, 1, 0, 0, 0, 0, 1, 0})If the implementation also needs adjustment to use “most recent,” update helper/MaxSince accordingly:
func MaxSince[T Number](c <-chan T, w int) <-chan T {
- return Window(c, func(w []T, i int) T {
- since := 0
- found := false
- m := slices.Max(w)
- SlicesReverse(w, i, func(n T) bool {
- if found && n < m {
- return false
- }
- since++
- if n == m {
- found = true
- }
- return true
- })
- return T(since - 1)
- }, w)
+ return Window(c, func(w []T, i int) T {
+ m := slices.Max(w)
+ var since int
+ SlicesReverse(w, i, func(n T) bool {
+ if n == m { // stop at first (most recent) occurrence of the max
+ return false
+ }
+ since++
+ return true
+ })
+ return T(since)
+ }, w)
}To align on definition, please run a quick web check:
🌐 Web query:
For the Aroon indicator, when multiple bars share the highest high within the lookback window, should "periods since highest high" be measured from the most recent occurrence or the earliest? Cite authoritative sources (e.g., TA-Lib, TradingView, StockCharts).
💡 Result:
When multiple bars share the highest high within the Aroon indicator's lookback window, "periods since highest high" should be measured from the most recent occurrence (i.e., the most recent bar with the highest high).
Authoritative sources support this as follows:
-
StockCharts states: “Aroon-Up...measures the number of periods since price recorded an x-day high” and provides the formula:
[
\text{Aroon-Up} = \left(\frac{N - \text{Days since } N\text{-day High}}{N}\right) \times 100
]
Their context, examples, and charts show that if the current bar equals the highest high of the window, "days since" is 0, regardless of earlier occurrences of the same high. Thus, "days since high" always refers to the most recent occurrence within the period[1][4]. -
TradingView (official Help Center):
“If the highest value occurs in the current candlestick, the up value is set to 100, indicating a new peak. If not, it returns a percentage value indicating the time elapsed since the last peak.” Here, “last peak” means the most recent occurrence within the window[3][7]. -
LightningChart provides a table and calculation:
"Periods since High within n = How many time periods since the highest High value within the specified time period count (n)" and in their example, when the highest value appears multiple times, the calculation uses the distance from the latest occurrence[9]. -
TA-Lib follows the same convention. Its public formulas do not distinguish between first or last occurrence, only "periods since," which is universally interpreted as periods since the most recent occurrence up to and including the current bar[10].
Summary Table: "Periods Since Highest High"
| Scenario | Value Used in Calculation |
|---|---|
| Highest high is on current bar | 0 |
| Highest high occurred n bars ago | n (counted from current bar to most recent high) |
| Multiple bars with same highest high value | Distance from current bar to most recent high bar |
Key sources:
- StockCharts[1][4]
- TradingView[3][7]
- LightningChart[9]
- TA-Lib (documentation and industry convention)[10]
No authoritative source (including StockCharts, TradingView, and TA-Lib) suggests using the earliest (oldest) occurrence within the lookback window. The industry standard and all major implementations measure from the most recent occurrence of the highest high.
Citations:
- 1: https://chartschool.stockcharts.com/table-of-contents/technical-indicators-and-overlays/technical-indicators/aroon
- 2: https://www.backtrader.com/docu/talib/talib/
- 3: https://www.tradingview.com/chart/NFLX/9oiL4w3t-Aroon-Indicator-Identifying-Trends-and-Momentum/
- 4: https://chartschool.stockcharts.com/table-of-contents/technical-indicators-and-overlays/technical-indicators/aroon-oscillator
- 5: https://www.avatrade.com/education/technical-analysis-indicators-strategies/aroon-indicator-strategies
- 6: https://technical-analysis-library-in-python.readthedocs.io/en/latest/ta.html
- 7: https://www.tradingview.com/support/solutions/43000501801-aroon/
- 8: https://chartschool.stockcharts.com/table-of-contents/technical-indicators-and-overlays/technical-indicators
- 9: https://lightningchart.com/blog/aroon-technical-indicator/
- 10: https://ta-lib.github.io/ta-lib-python/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching repo for MaxSince symbols..."
rg -n --hidden "MaxSince" || true
echo
echo "Searching for files named *max*since*..."
fd -HI -e go max | sed -n '1,200p' || true
echo
echo "Showing helper/max_since_test.go (if exists):"
if [ -f helper/max_since_test.go ]; then
sed -n '1,200p' helper/max_since_test.go
else
echo "helper/max_since_test.go not found"
fi
echo
echo "Showing helper/max_since.go (if exists):"
if [ -f helper/max_since.go ]; then
sed -n '1,240p' helper/max_since.go
else
echo "helper/max_since.go not found"
fi
echo
echo "Searching for MaxSince implementation (func MaxSince):"
rg -n --hidden "func\s+MaxSince" -S || true
echo
echo "Searching for SlicesReverse or similar helpers used in original review suggestion..."
rg -n --hidden "SlicesReverse|slices.Max|Window(" -S || trueLength of output: 3024
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for SlicesReverse and Window definitions..."
rg -n --hidden "func\s+SlicesReverse|func\s+Window" -S || true
echo
echo "Searching for SlicesReverse usages to locate its file..."
rg -n --hidden "SlicesReverse" -S || true
echo
echo "Showing files that likely contain these helpers (helper/*.go):"
ls -1 helper/*.go || true
echo
echo "Printing helper/slices_reverse.go and helper/window.go if present:"
for f in helper/*; do
case "$(basename "$f")" in
*slice*|*reverse*|*window*|*helpers*|*util*|*slices*)
echo "---- $f ----"
sed -n '1,240p' "$f" || true
echo
;;
esac
done
echo
echo "Printing all helper/*.go for manual scan (first 400 lines each):"
for f in helper/*.go; do
echo "---- $f ----"
sed -n '1,400p' "$f" || true
echo
doneLength of output: 156076
MaxSince/MinSince use the earliest tie-break today — change to the most-recent occurrence (Aroon convention)
Confirmed: helper/max_since.go and helper/min_since.go currently pick the earliest occurrence among tied extrema; authoritative sources (StockCharts / TradingView / TA‑Lib) define "periods since high/low" as distance to the most recent occurrence. Update both implementations and their tests.
Files to change:
- helper/max_since.go
- helper/max_since_test.go
- helper/min_since.go
- helper/min_since_test.go
Suggested diffs:
- helper/max_since_test.go
- expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 0, 1, 0})
+ expected := SliceToChan([]int{0, 0, 1, 0, 0, 0, 0, 1, 0})- helper/max_since_test.go (TestMaxSinceAtEnd)
- expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 2})
+ expected := SliceToChan([]int{0, 0, 1, 0, 0, 0, 0})- helper/max_since.go
func MaxSince[T Number](c <-chan T, w int) <-chan T {
- return Window(c, func(w []T, i int) T {
- since := 0
- found := false
- m := slices.Max(w)
- SlicesReverse(w, i, func(n T) bool {
- if found && n < m {
- return false
- }
- since++
- if n == m {
- found = true
- }
- return true
- })
- return T(since - 1)
- }, w)
+ return Window(c, func(w []T, i int) T {
+ m := slices.Max(w)
+ var since int
+ // stop at the first (most recent) occurrence of the max
+ SlicesReverse(w, i, func(n T) bool {
+ if n == m {
+ return false
+ }
+ since++
+ return true
+ })
+ return T(since)
+ }, w)
}- helper/min_since_test.go
- expected := SliceToChan([]int{0, 1, 2, 2, 0, 1, 2})
+ expected := SliceToChan([]int{0, 1, 2, 0, 0, 0, 1})- helper/min_since_test.go (TestMinSinceAtEnd)
- expected := SliceToChan([]int{0, 1, 2, 2, 0, 1, 2})
+ expected := SliceToChan([]int{0, 1, 2, 0, 0, 0, 0})- helper/min_since_test.go (TestMinSinceFromStart)
- expected := SliceToChan([]int{0, 1, 2})
+ expected := SliceToChan([]int{0, 0, 1})- helper/min_since.go
func MinSince[T Number](c <-chan T, w int) <-chan T {
- return Window(c, func(w []T, i int) T {
- since := 0
- found := false
- m := slices.Min(w)
- SlicesReverse(w, i, func(n T) bool {
- if found && n > m {
- return false
- }
- since++
- if n == m {
- found = true
- }
- return true
- })
- return T(since - 1)
- }, w)
+ return Window(c, func(w []T, i int) T {
+ m := slices.Min(w)
+ var since int
+ // stop at the first (most recent) occurrence of the min
+ SlicesReverse(w, i, func(n T) bool {
+ if n == m {
+ return false
+ }
+ since++
+ return true
+ })
+ return T(since)
+ }, w)
}Please apply these changes (or confirm if your intended tie-break is different). After updating, run the test suite — Aroon-related code (trend/aroon.go) is consistent with the most-recent semantics and should produce correct Aroon values thereafter.
📝 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 TestMaxSince(t *testing.T) { | |
| input := SliceToChan([]int{48, 49, 47, 52, 52, 52, 53, 50, 55}) | |
| expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 0, 1, 0}) | |
| actual := MaxSince(input, 3) | |
| err := CheckEquals(actual, expected) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| func TestMaxSince(t *testing.T) { | |
| input := SliceToChan([]int{48, 49, 47, 52, 52, 52, 53, 50, 55}) | |
| expected := SliceToChan([]int{0, 0, 1, 0, 0, 0, 0, 1, 0}) | |
| actual := MaxSince(input, 3) | |
| err := CheckEquals(actual, expected) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In helper/max_since_test.go around lines 7-16, the test and implementation
currently assume earliest-occurrence tie-breaking; change both
helper/max_since.go and helper/min_since.go to break ties by choosing the
most-recent occurrence (largest index) instead of the earliest, and update all
related tests (helper/max_since_test.go, helper/max_since_test.go
TestMaxSinceAtEnd, helper/min_since_test.go, helper/min_since_test.go
TestMinSinceAtEnd, helper/min_since_test.go TestMinSinceFromStart) so expected
outputs reflect distance to the most recent high/low; specifically, where the
code currently selects the first equal max/min index, change logic to update the
chosen index when a tied value is encountered (prefer later index), and adjust
test expected SliceToChan arrays accordingly, then run the test suite to verify
Aroon-related outputs remain correct.
| func TestMaxSinceAtEnd(t *testing.T) { | ||
| input := SliceToChan([]int{48, 49, 47, 52, 52, 52, 52}) | ||
| expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 2}) | ||
| actual := MaxSince(input, 3) | ||
|
|
||
| err := CheckEquals(actual, expected) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue in TestMaxSinceAtEnd: duplicates at the end should produce 0 if ties resolve to “most recent”
In a window [52, 52, 52], the distance to the last (most recent) high is 0, not 2. Adjust expected accordingly if the intended tie-break is “most recent.”
Apply:
- expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 2})
+ expected := SliceToChan([]int{0, 0, 1, 0, 0, 0, 0})📝 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 TestMaxSinceAtEnd(t *testing.T) { | |
| input := SliceToChan([]int{48, 49, 47, 52, 52, 52, 52}) | |
| expected := SliceToChan([]int{0, 0, 1, 0, 1, 2, 2}) | |
| actual := MaxSince(input, 3) | |
| err := CheckEquals(actual, expected) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| } | |
| func TestMaxSinceAtEnd(t *testing.T) { | |
| input := SliceToChan([]int{48, 49, 47, 52, 52, 52, 52}) | |
| expected := SliceToChan([]int{0, 0, 1, 0, 0, 0, 0}) | |
| actual := MaxSince(input, 3) | |
| err := CheckEquals(actual, expected) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| } |
🤖 Prompt for AI Agents
In helper/max_since_test.go around lines 18 to 27, the test's expected output
assumes ties pick the oldest max but the intended tie-break is “most recent”;
update the expected channel to reflect that duplicates at the end produce 0
(change the expected SliceToChan from []int{0,0,1,0,1,2,2} to
[]int{0,0,1,0,1,0,0}) so the test verifies ties resolve to the most recent
occurrence.
Describe Request
Introduce Window, MaxSince and MinSince on channels to hopefully fix #124 as suggested.
Fixed # (issue)
Change Type
What is the type of this change.
Summary by CodeRabbit
New Features
Bug Fixes
Tests