fix the calculation of the WMA #279
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (2)
Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. WalkthroughUpdates WMA computation to use weights N..1 with proper normalization (divisor N*(N+1)/2), adds input validation that panics when period ≤ 0, and expands tests to validate WMA for periods 3 and 5. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant New as NewWmaWith
participant Wma as Wma[T]
participant Compute as Compute
participant Ring as Ring[T]
participant Helper as helper (Map/Skip)
Caller->>New: NewWmaWith(period)
alt period <= 0
New-->>Caller: panic("period must be > 0")
else
New-->>Caller: Wma instance
end
Caller->>Compute: Compute(values <-chan T)
Compute->>Ring: NewRing(period)
Compute->>Helper: Map(values, fn)
loop for each value
Helper->>Compute: value
Compute->>Ring: Put(value)
alt Ring not full
Compute-->>Helper: emit 0
else Ring full
Note right of Compute: divisor = N*(N+1)/2
Compute->>Ring: At(i) for i=0..N-1
Note right of Compute: sum += window.At(i) * (N - i)
Compute-->>Helper: emit sum/divisor
end
end
Compute->>Helper: Skip(wmas, IdlePeriod)
Helper-->>Caller: <-chan T
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
This fix cinar#274, WMA calculation was wrong, correct formular is, i.e. WMA(5): (P1 * 5) + (P2 * 4) + (P3 * 3) + (P4 * 2) + (P5 * 1) / (5 + 4 + 3 + 2 + 1). This makes it perfect for the divisor to be calulated only one (gauss formula: (n * (n + 1) / 2). The testdata has been new caclulated manually via Excel, a WMA(3) and a WMA(5) … to be safe. Signed-off-by: Heiko Weber <[email protected]>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
trend/wma.go (1)
13-17: Doc formula is incorrect and conflicts with the new implementation.Update to the standard triangular-weighted form.
-// -// WMA = ((Value1 * 1/N) + (Value2 * 2/N) + ...) / 2 +// +// WMA = (P1*N + P2*(N-1) + ... + PN*1) / (N*(N+1)/2)
🧹 Nitpick comments (4)
trend/wma.go (2)
35-35: Minor literal nit: use an integer literal for 2.Keeps conversions cleaner across T.
-divisor := T(w.Period) * (T(w.Period) + T(1)) / T(2.0) +divisor := T(w.Period) * (T(w.Period) + T(1)) / T(2)
50-50: Guard against integer instantiation of T (precision loss).If helper.Number allows ints, sum/divisor will truncate. Either constrain T to floats or do the division in float and cast back.
Option A (preferred): restrict T to floats in this indicator.
// in this file or via a shared constraint type floatish interface{ ~float32 | ~float64 } type Wma[T floatish] struct{ Period int }Option B: compute in float64 and cast (only if T is convertible to float64 across the codebase).
- return sum / divisor + return T(float64(sum) / float64(divisor))trend/wma_test.go (2)
38-39: Rounding precision check.Ensure testdata/wma.csv values are at 3 decimals; otherwise, round expected too for strict equality.
-actualWma3 = helper.RoundDigits(actualWma3, 3) -actualWma5 = helper.RoundDigits(actualWma5, 3) +actualWma3 = helper.RoundDigits(actualWma3, 3) +actualWma5 = helper.RoundDigits(actualWma5, 3) +// if CSV has more precision, also round expected: +// expectedWma3 = helper.RoundDigits(expectedWma3, 3) +// expectedWma5 = helper.RoundDigits(expectedWma5, 3)
41-45: Verify helper.CheckEquals signature for multi-pair comparison.If CheckEquals doesn’t accept 4 streams (two pairs), split into two asserts.
-err = helper.CheckEquals(actualWma3, expectedWma3, actualWma5, expectedWma5) +if err = helper.CheckEquals(actualWma3, expectedWma3); err != nil { + t.Fatal(err) +} +if err = helper.CheckEquals(actualWma5, expectedWma5); 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/wma.csvis excluded by!**/*.csv
📒 Files selected for processing (2)
trend/wma.go(3 hunks)trend/wma_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
trend/wma_test.go (4)
helper/csv.go (1)
ReadFromCsvFile(310-317)trend/wma.go (1)
NewWmaWith(23-30)helper/skip.go (1)
Skip(15-30)trend/vwma_test.go (1)
TestVwma(14-42)
🔇 Additional comments (5)
trend/wma.go (2)
24-26: Input validation LGTM.Panic on non-positive period is consistent and keeps invalid configs out early.
46-48: Confirm ring ordering to ensure newest gets the highest weight.This assumes window.At(0) returns the most recent value (weight N), decreasing to 1 at At(N-1). If At(0) is the oldest, the weights are reversed.
If At(0) is oldest, switch to:
- v := window.At(i) - sum += v * T(w.Period-i) + v := window.At(i) + sum += v * T(i+1) // oldest=1 ... newest=Ntrend/wma_test.go (3)
14-14: Clear test description. LGTM.
18-20: Schema aligns with CSV-driven expectations.Fields Wma3/Wma5 mirror per-period expectations.
27-30: Channel duplication/map setup looks good.Efficiently tees Close into two streams; expected channels sourced independently.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Heiko Weber <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
==========================================
- Coverage 91.81% 91.79% -0.03%
==========================================
Files 191 191
Lines 6794 6797 +3
==========================================
+ Hits 6238 6239 +1
- Misses 493 494 +1
- Partials 63 64 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
Describe Request
This fix #274, WMA calculation, which formulas wasn't correct implemented.
Fixed #274
Change Type
What is the type of this change.
Correct formular of WMA is, i.e. WMA(5):
(P1 * 5) + (P2 * 4) + (P3 * 3) + (P4 * 2) + (P5 * 1) / (5 + 4 + 3 + 2 + 1).This makes it perfect for the divisor to be calulated only one (gauss formula:
(n * (n + 1) / 2).The testdata has been new caclulated manually via Excel, a WMA(3) and a WMA(5) … to be safe.
Summary by CodeRabbit
Bug Fixes
Improvements
Tests