Skip to content

Improve Weighted Moving Average (WMA) indicator parity with Cython #2662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 25, 2025

Conversation

nicolad
Copy link
Collaborator

@nicolad nicolad commented May 22, 2025

Restores feature-parity and behavioural equivalence between the Rust WeightedMovingAverage implementation and the canonical Python/Cython reference.


1 · Why this matters — Context & Motivation

Theme Why it matters
Correctness ⇄ Consistency Mixed Python (quant research) + Rust/WASM (execution) stacks must emit identical WMA values; silent divergence leaks P ∿ L.
Robust edge-case handling Zero-weight, sub-ε weight-sum or NaN inputs previously produced UB or silent drift.
Simpler mental model Removes the redundant has_inputs flag and double-initialisation logic.
Test-driven culture Full-parity rstest suite protects against future regressions and enables confident refactors.

2 · What changed

Concern Rust before Rust after (this PR)
Validation rules Sparse, panicked on some edge cases only ✨ Full validation in new_checked: period > 0, weights.len() == period, ∑wᵢ > ε
Redundant state has_inputs boolean tracked manually Removedhas_inputs() now derives from !inputs.is_empty()
Weighted sum loop Manual index + reverse_weights vec Direct zip(self.inputs.iter().rev(), self.weights.iter().rev()) (zero alloc)
Initialisation flag Two competing assignments Single canonical rule: initialized = count() ≥ period
Error messages Generic Descriptive, actionable ("period must equal weights.len()")
Unit tests 8 cases 30 cases / 100 % line & branch coverage
Docs Minimal Elaborate /// comments on invariants & panics
Performance note No observable regression (same O(1) update); future SIMD possible ✈️

3 · Implementation notes

  • Validation constant EPS = f64::EPSILON ensures weight-sum check does not break on FP rounding.
  • weighted_average() is safe because self.weights.len() == self.period is proven by construction.
  • Negative weights are tolerated iff the total is positive (common in certain signal schemes); covered by tests.
  • NaN inputs poison the output until reset() — matches Python behaviour.
  • WeightedMovingAverage is still !Send + !Sync; wrap in Arc<Mutex<…>> when sharing between threads.

4 · Tests added / updated

Test name Purpose
new_panics_on_zero_period / new_checked_err_on_zero_period Guards period > 0
new_panics_on_zero_weight_sum / weight_sum_below_epsilon variants Enforces ∑wᵢ > ε
initialized_flag_transitions Canonical initialisation after period samples
count_matches_inputs_and_has_inputs Verifies redundant flag removal
weighted_average_with_non_uniform_weights Numerical accuracy with 1:2:3 weights
negative_weights_positive_sum Mixed-sign weights sanity
nan_input_propagates NaN poisoning rule
single_period_returns_latest_input period == 1 degenerates to last price
value_with_sparse_weights Sparse weight-vector edge case
+ 20 more robustness cases30 total / 100 % coverage.

Related #2507

@nicolad nicolad requested a review from cjdsellers May 22, 2025 19:22
@nicolad nicolad self-assigned this May 22, 2025
@nicolad nicolad marked this pull request as ready for review May 23, 2025 06:56
@nicolad
Copy link
Collaborator Author

nicolad commented May 23, 2025

needs some attention from you @cjdsellers 👇

# Weight rule / behaviour Rust WMA (new_checked) Python WMA Parity
1 weights.len() == period ✔ Enforced (check_predicate_true) ✔ Enforced
2 Σ weights > ε (f64::EPSILON / np.finfo(float).eps) ✔ Enforced ✔ Enforced
3 Negative weights allowed if Σ > ε ✔ Allowed ✔ Allowed
4 Weights optional (None) → SMA fallback ✖ Not supported (must pass Vec<f64>) ✔ Supported (weights=None) ⚠️
5 Warm-up uses trailing subset of weights (weights[-n:]) ✔ Yes (weights[self.period-n..]) ✔ Yes
6 Guard that warm-up Σ weights > ε ✖ Open ✖ Open ⚠️
7 Invalid weights ⇒ runtime error panic! (in new), Err (checked) ValueError
8 Degenerate case period=1, weights=[1.0] → passthrough ✔ Works ✔ Works
9 NaN inside weights detected / rejected ✖ No explicit guard (Σ check catches) ✖ Same

Copy link
Member

@cjdsellers cjdsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff, and thanks for all the tests!

Removing has_inputs is a nice refinement.

On the suggestions, I don't think we need to support optional weights at this stage?

if self.inputs.len() == self.period {
self.inputs.remove(0);
self.inputs.pop_front();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're using a VecDeque with a fixed capacity then this might not be necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VecDeque::with_capacity(n) only pre-allocates space; it doesn’t enforce a hard cap.
If you keep pushing after len == n, the deque re-allocates and grows, so you must still pop_front() (or otherwise drop the oldest element) to keep the window at exactly period items.

@nautechsystems nautechsystems deleted a comment from CLAassistant May 24, 2025
@nicolad nicolad requested a review from cjdsellers May 24, 2025 04:48
@cjdsellers cjdsellers merged commit 2d0bb45 into develop May 25, 2025
17 checks passed
@cjdsellers cjdsellers deleted the 2507-wma branch May 25, 2025 06:14
Copy link
Member

@cjdsellers cjdsellers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @nicolad 👌

@nicolad nicolad added the rust Relating to the Rust core label May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Relating to the Rust core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants