Conversation
WalkthroughAdds helper.SkipLast for streaming channels and implements a generic DPO (Detrended Price Oscillator) indicator with constructors, Compute (stream-based using Duplicate, SMA, Skip/SkipLast, Operate), IdlePeriod, String, and unit tests for both helper and trend packages. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Caller
participant DPO as Dpo.Compute
participant Dup as helper.Duplicate
participant SMA as SMA
participant Skip as helper.Skip / helper.SkipLast
participant Oper as helper.Operate
participant OUT as Output
Caller->>DPO: pass closing <-chan T
DPO->>Dup: Duplicate(closing)
Dup-->>DPO: streamA, streamB
DPO->>SMA: Compute SMA over streamA
SMA-->>DPO: smaStream
DPO->>Skip: Skip (idle) on streamB and SkipLast on smaStream to align
Skip-->>DPO: alignedPrice, shiftedSMA
DPO->>Oper: Operate(alignedPrice, shiftedSMA, subtract)
Oper-->>OUT: emit DPO values
Note over OUT: output closes when input completes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
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. 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 |
|
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: 1
🧹 Nitpick comments (6)
helper/skip_last.go (2)
11-19: Clamp negative count to avoid surprising semantics.Today, a negative
countacts like0(always sends) due tolen(buf) > count. Be explicit to match intent and guard future refactors.func SkipLast[T any](c <-chan T, count int) <-chan T { - result := make(chan T, cap(c)) + if count < 0 { + count = 0 + } + result := make(chan T, cap(c))
14-31: Minor: fast-path for count==0 to avoid buffering.Optional micro-optimization: when
count==0, just pipe-through.go func() { defer close(result) - // Buffer to hold the last "count" elements + if count == 0 { + Pipe(c, result) + return + } + // Buffer to hold the last "count" elements buf := make([]T, 0, count)helper/skip_last_test.go (1)
13-23: LGTM; add a couple of edge cases.Consider adding tests for
count == 0(pass-through) andcount >= len(input)(drops all).func TestSkipLast(t *testing.T) { @@ } + +func TestSkipLast_Zero(t *testing.T) { + input := helper.SliceToChan([]int{1, 2, 3}) + actual := helper.SkipLast(input, 0) + expected := helper.SliceToChan([]int{1, 2, 3}) + if err := helper.CheckEquals(actual, expected); err != nil { + t.Fatal(err) + } +} + +func TestSkipLast_DropsAll(t *testing.T) { + input := helper.SliceToChan([]int{1, 2, 3}) + actual := helper.SkipLast(input, 5) + // Expect empty + expected := helper.SliceToChan([]int{}) + if err := helper.CheckEquals(actual, expected); err != nil { + t.Fatal(err) + } +}trend/dpo.go (2)
67-70: Minor: clarify IdlePeriod docstring and reuse k.Small readability win; also avoids recomputing
d.Period/2 + 1.-// IdlePeriod is the initial period that DPO yield any results. +// IdlePeriod returns the number of initial input values to drop before DPO yields results. func (d *Dpo[T]) IdlePeriod() int { - return (d.Period - 1) + (d.Period/2 + 1) + k := d.Period/2 + 1 + return (d.Period - 1) + k }
57-60: Nit: compute k once and reuse.Avoid repeating
d.Period/2 + 1.- skippedClosing := helper.Skip(closingSplice[1], d.IdlePeriod()) - smaDelayed := helper.SkipLast(smaOut, d.Period/2+1) + k := d.Period/2 + 1 + skippedClosing := helper.Skip(closingSplice[1], (d.Period-1)+k) + smaDelayed := helper.SkipLast(smaOut, k)trend/dpo_test.go (1)
25-33: Good coverage; consider adding a small handcrafted series test.A short, hand-computed sequence (e.g., 8–10 points) would help pin down the chosen DPO convention and guard against alignment regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
trend/testdata/dpo.csvis excluded by!**/*.csv
📒 Files selected for processing (4)
helper/skip_last.go(1 hunks)helper/skip_last_test.go(1 hunks)trend/dpo.go(1 hunks)trend/dpo_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
trend/dpo.go (2)
helper/skip.go (1)
Skip(15-30)helper/skip_last.go (1)
SkipLast(11-32)
trend/dpo_test.go (3)
trend/dpo.go (3)
Dpo(28-30)NewDpo(33-37)NewDpoWithPeriod(40-46)helper/csv.go (1)
ReadFromCsvFile(310-317)helper/skip.go (1)
Skip(15-30)
helper/skip_last_test.go (1)
helper/skip_last.go (1)
SkipLast(11-32)
helper/skip_last.go (3)
helper/skip.go (1)
Skip(15-30)helper/shift.go (1)
Shift(15-27)helper/sync.go (1)
SyncPeriod(29-37)
🔇 Additional comments (3)
helper/skip_last.go (1)
11-31: Solid implementation; windowing logic is correct and safe under backpressure.The sliding buffer correctly drops the tail and preserves order; channel lifetime is handled via defer close.
trend/dpo.go (1)
48-65: Double-check DPO alignment (off-by-k risk).Typical DPO: k = floor(period/2)+1; DPO(t) is often defined as Price(t−k) − SMA_n(t), equivalently Price(t) − SMA_n(t+k). The current pairing computes
price(t) - sma(t-k)due toSkipLaston SMA andSkipon price byIdlePeriod. Please verify against the reference dataset and confirm intended alignment.If the goal is Price(t) − SMA(t+k), consider shifting SMA “forward” by k instead:
func (d *Dpo[T]) Compute(closing <-chan T) <-chan T { - closingSplice := helper.Duplicate(closing, 2) + closingDup := helper.Duplicate(closing, 2) + k := d.Period/2 + 1 // compute SMA on the first duplicated stream sma := NewSma[T]() sma.Period = d.Period - smaOut := sma.Compute(closingSplice[0]) + smaOut := sma.Compute(closingDup[0]) - // align the original price stream and the SMA stream according to DPO formula - skippedClosing := helper.Skip(closingSplice[1], d.IdlePeriod()) - smaDelayed := helper.SkipLast(smaOut, d.Period/2+1) + // align streams: skip SMA's warmup and then skip an additional k on SMA + smaShifted := helper.Skip(smaOut, k) // S(t+k) + price := helper.Skip(closingDup[1], d.Period-1) // Price(t) // DPO = Price - shifted SMA - return helper.Operate(skippedClosing, smaDelayed, func(price, shiftedSma T) T { - return price - shiftedSma + return helper.Operate(price, smaShifted, func(p, s T) T { + return p - s }) }If this change alters test expectations, the CSV or alignment step in tests may reflect a different convention. Please confirm which convention the project standardizes on.
trend/dpo_test.go (1)
35-41: LGTM on String().Naming and output format match expectations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
trend/dpo.go (1)
39-48: Input validation in NewDpoWithPeriod looks good.
Fallback to default when period < 1 addresses earlier review feedback; allowing period=1 is reasonable.
🧹 Nitpick comments (4)
trend/dpo.go (4)
16-27: Clarify formula comment and example to match implementation.
Current wording “SMA(Price shifted by k)” can be read as recomputing SMA on a shifted series. The code computes Price[t] − SMA[t−k]. Also prefer showcasing NewDpoWithPeriod in the example.-// Formula (common approximation): -// -// k = period/2 + 1 -// DPO = Price - SMA(Price shifted by k) +// Formula (common approximation): +// Let k = floor(period/2) + 1. +// For time index t >= period-1+k: +// DPO[t] = Price[t] - SMA[t - k] @@ -// dpo := trend.NewDpo[float64]() -// dpo.Period = 20 +// dpo := trend.NewDpoWithPeriod[float64](20) // out := dpo.Compute(c)
28-30: Document the Period contract (>= 1) to prevent invalid states.
Since Period is exported, callers can still set invalid values post-construction. At minimum, document it here.type Dpo[T helper.Number] struct { - Period int + // Period is the SMA window length. Must be >= 1. Typical default is 20. + Period int }
51-62: Minor readability: factor k and rename local for clarity.
Reduces repeated integer math and avoids the slightly confusing “closingSplice” name.-func (d *Dpo[T]) Compute(closing <-chan T) <-chan T { - closingSplice := helper.Duplicate(closing, 2) +func (d *Dpo[T]) Compute(closing <-chan T) <-chan T { + k := d.Period/2 + 1 + dup := helper.Duplicate(closing, 2) @@ - smaOut := sma.Compute(closingSplice[0]) + smaOut := sma.Compute(dup[0]) @@ - skippedClosing := helper.Skip(closingSplice[1], d.IdlePeriod()) - smaDelayed := helper.SkipLast(smaOut, d.Period/2+1) + skippedClosing := helper.Skip(dup[1], d.IdlePeriod()) + smaDelayed := helper.SkipLast(smaOut, k)
69-72: Polish the IdlePeriod doc.
Small grammar cleanup; clarifies intent.-// IdlePeriod is the initial period that DPO yield any results. +// IdlePeriod returns the number of leading samples to discard before the first DPO value is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
trend/dpo.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
trend/dpo.go (1)
helper/skip.go (1)
Skip(15-30)
🔇 Additional comments (4)
trend/dpo.go (4)
13-14: LGTM: Sensible default period.
Good constant; matches common defaults for DPO.
32-37: LGTM: Default constructor.
Returns a valid DPO with a sane default.
50-67: LGTM: Alignment and stream lengths are correct.
The Skip/SkipLast pairing yields equal lengths: (M − (N−1) − k) on both sides; no goroutine leaks or misalignment. Nice.
75-77: LGTM: String representation.
Matches project conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
trend/dpo.go (1)
27-30: Doc consistency: clarify valid period and constructor fallback.The struct comment says “Must be >= 1” but NewDpoWithPeriod clamps period <= 1 to DefaultDpoPeriod. Make docs explicit to avoid confusion.
Apply this doc-only diff:
type Dpo[T helper.Float] struct { - // Period is the SMA window length. Must be >= 1. Typical default is 20. + // Period is the SMA window length. Typical default is 20. + // Note: values <= 1 are considered invalid and will be replaced with DefaultDpoPeriod by constructors. Period int } -// NewDpoWithPeriod function initializes a new DPO instance with the given period. +// NewDpoWithPeriod initializes a new DPO instance with the given period. +// Periods <= 1 are clamped to DefaultDpoPeriod. func NewDpoWithPeriod[T helper.Float](period int) *Dpo[T] { if period <= 1 { period = DefaultDpoPeriod }Also applies to: 39-44
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
trend/dpo.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
trend/dpo.go (2)
helper/skip.go (1)
Skip(15-30)helper/skip_last.go (1)
SkipLast(11-32)
🔇 Additional comments (3)
trend/dpo.go (3)
16-26: LGTM: Formula and alignment approach are correct.Using k = floor(period/2) + 1 and aligning streams via Skip/SkipLast matches the common DPO definition.
27-27: Good call on restricting to helper.Float.Prevents integer truncation in SMA/DPO math.
76-78: LGTM: String() formatting is idiomatic and clear.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #281 +/- ##
==========================================
+ Coverage 91.81% 91.83% +0.01%
==========================================
Files 183 185 +2
Lines 6672 6721 +49
==========================================
+ Hits 6126 6172 +46
- Misses 486 488 +2
- Partials 60 61 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
helper/README.md (1)
953-969: Specify fenced code languages and clarify edge-case behavior
- Add languages to fenced blocks to satisfy MD040 and improve readability.
- Suggest documenting behavior for count <= 0 and count >= stream length (no-op vs. empty), and that output channel capacity equals cap(c) with asynchronous goroutine.
Apply this diff to add languages:
-``` +```go func SkipLast[T any](c <-chan T, count int) <-chan T-
+go
c := helper.SliceToChan([]int{2, 4, 6, 8})
actual := helper.SkipLast(c, 2)
fmt.Println(helper.ChanToSlice(actual)) // [2, 4]Optionally extend the description with a brief note:
SkipLast skips the specified number of elements from the end of the given channel. +If count <= 0, the input is passed through unchanged. If count is greater than or +equal to the number of input elements, the output is empty. The function returns +immediately; a goroutine drains the input and writes to the output channel (buffered +with cap(c)).trend/README.md (2)
308-313: Minor consistency: group DefaultDpoPeriod with other defaultsOther defaults are shown in grouped const blocks. Consider emitting DefaultDpoPeriod from a grouped const in code so gomarkdoc renders it consistently.
627-693: Add fenced code languages; small doc polish and verification
- Add languages to fenced blocks (MD040): use “text” for the formula and “go” for the example.
- Optional: state IdlePeriod explicitly as (period-1) + (floor(period/2)+1).
- Verify doc matches implementation: “Periods <= 1 are clamped to DefaultDpoPeriod.” If 1 is supported, change to “< 1”.
Apply this diff to fix fenced languages:
Dpo computes the Detrended Price Oscillator. Formula (common approximation): Let k = floor(period/2) + 1. For time index t >= period-1+k: -``` +```text DPO[t] = Price[t] - SMA[t - k]Example:
-
+go
dpo := trend.NewDpoWithPeriodfloat64
out := dpo.Compute(c)If IdlePeriod is fixed and useful to expose in docs:
IdlePeriod returns the number of leading samples to discard before the first DPO value is available. +It equals (Period - 1) + (floor(Period/2) + 1).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)helper/README.md(2 hunks)trend/README.md(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
helper/README.md (1)
helper/skip.go (1)
Skip(15-30)
🪛 markdownlint-cli2 (0.17.2)
helper/README.md
964-964: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
trend/README.md
632-632: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
638-638: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (3)
README.md (1)
37-37: Add DPO to the Trend list — looks goodLink and anchor format match the project’s convention.
helper/README.md (1)
82-82: Index entry for SkipLast — OKAnchor and naming are consistent with existing helper functions.
trend/README.md (1)
46-51: DPO added to the API index — looks consistentType parameter bound (helper.Float) aligns with other oscillators (e.g., ROC).
3 participants
Describe Request
Add trend indicator DPO
Fixed #51
Change Type
What is the type of this change.
Summary by CodeRabbit
New Features
Tests
Documentation