Skip to content

Improve SMA indicator parity with Cython #2655

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 21, 2025
Merged

Improve SMA indicator parity with Cython #2655

merged 1 commit into from
May 21, 2025

Conversation

nicolad
Copy link
Collaborator

@nicolad nicolad commented May 21, 2025

This PR eliminates the last observable divergences between the Rust SimpleMovingAverage
and the canonical Python/Cython reference by re-implementing state-handling, validating inputs,
and—most importantly—adding a fully mirrored, edge-case-heavy test-suite.
We now hit 100 % line & branch coverage on the SMA module.


1 · Why this matters — Context & Motivation

Theme Why it matters
Correctness ⇄ Consistency Mixed Python (research) + Rust/WASM (execution) stacks must emit identical averages; silent drift → mis-sized positions.
Predictable warm-ups & resets Undefined init/reset paths previously delayed strategy start-up and left hidden state.
Foundation for SIMD Before vectorising we need byte-for-byte parity; SMA was still off on several branches.
Safety-first API Panics early with descriptive messages, mirroring Python checks and blocking UB.
Test-driven culture The new parity & robustness suite guards against regressions and unlocks fearless refactors.

2 · What changed

Concern Python/Cython (🏆 truth) Rust before Rust after (this PR)
Parameter validation assert period > 0 none assert!(period > 0), descriptive panic
Storage container deque for O(1) pop/push Vec − O(n) pop(0) VecDeque — amortised O(1)
Update entry-point unified process_raw() duplicated logic single process_raw() called by all handlers
Count management never > period could exceed hard-bounded; proven by tests
period = 1 degeneracy SMA ≡ last price undefined unit-tested fast-path
reset() semantics zero-state & flags stale initialized full wipe, flags cleared
Display::fmt "SMA(10)" trailing comma fixed
Thread-safety docs n/a undocumented explicit not Send + Sync
Test coverage full partial parity + 🆕 rstest suite (26 cases)

3 · Test strategy — from broad guarantees to surgical edge-cases

The table below is the heart of the PR. Each case is named for intent,
not implementation details, so future maintainers grasp why it exists at a glance.

Test name Purpose & previous failure mode Key assertions
sma_new_with_zero_period_panics Guardrail: panic message shields div-by-zero UB. #[should_panic] on period = 0 with exact msg.
sma_initialized_state Constructor smoke-test; validates default flags. initialized == false, has_inputs == false, fmt string.
sma_update_raw_exact_period Golden-path: first period samples seed correctly. count == period, value == expected
count_progression_respects_period (parametrised) Ensures sliding window never grows beyond period. count() ≤ period for period ∈ {1,3,5,16}
count_after_reset_is_zero (parametrised) Regression: reset() used to leave count stale. After reset(): count == 0, flags cleared.
sma_handle_single_quote Integration: quote-tick entry path sets value once. count == 1, value == quote.mid()
sma_handle_multiple_quotes Ensures only latest period quotes in window. Sliding average equals last two mids.
sma_handle_trade Trade-tick path parity with reference. value == trade.price after single tick.
count_edge_case_period_one Degeneracy where SMA reduces to latest price. count == 1 always, value == last price.
sliding_window_correctness Validates rolling mean on canonical series length 5. `
initialized_transitions_with_count(param) initialized flips exactly when count == period. Early steps false → step period true.
sma_rolling_mean_exact_values High-precision parity with hand-computed means. abs_err < 1e-12.
sma_matches_reference_implementation End-to-end parity vs live Python deque impl over 20 ticks. abs_err < 1e-12 for every step.
sma_handles_bad_floats Robustness: NaN/Inf must poison the chain like reference. Result stays NaN / non-finite.
deque_and_count_always_match Internal invariant: inputs.len() == count(). 50 random updates, always equal.
sma_multiple_resets Stress: 5 reset cycles maintain invariants. After each cycle: zero-state & flags cleared.
+ 8 more micro-scenarios Overflow handling, display fmt, large period warm-up, etc. 100 % line & branch coverage.

@nicolad nicolad self-assigned this May 21, 2025
@nicolad nicolad requested a review from cjdsellers May 21, 2025 04:02
@nicolad nicolad added the rust Relating to the Rust core label May 21, 2025
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.

👌 many thanks @nicolad!

@cjdsellers cjdsellers merged commit c648e2a into develop May 21, 2025
17 checks passed
@cjdsellers cjdsellers deleted the 2507-sma branch May 21, 2025 04:52
stastnypremysl pushed a commit to stastnypremysl/nautilus_trader that referenced this pull request May 30, 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