Skip to content

Change VecDeque for fixed-capacity ArrayDeque in SMA #2666

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 24, 2025

Conversation

nicolad
Copy link
Collaborator

@nicolad nicolad commented May 24, 2025

Why touch the buffer at all?

SimpleMovingAverage slides over a window whose length (period) is known up-front.
With the existing std::collections::VecDeque the queue can still grow past that length: each push_back that exceeds capacity triggers a heap re-allocation and mem-copy. VecDeque is explicitly documented as “a double-ended queue implemented with a growable ring buffer”.

Those reallocations are (a) wasted work—because we immediately pop_front afterwards—and (b) a latency spike on the trading hot-path.

arraydeque::ArrayDeque, on the other hand, is “a circular buffer with fixed capacity … [that] can be stored directly on the stack”.
Its capacity is a const generic baked into the type, so pushes never allocate and never exceed the logical window.


VecDeque vs ArrayDeque at a glance

Aspect VecDeque ArrayDeque<N>
Capacity Dynamic; grows geometrically on overflow Compile-time fixed (const N)
Allocation Heap (may reallocate) None (stack or static)
Push/Pop cost Amortised O(1) but occasional mem-copy Strict O(1)
Enforced upper bound Caller-dependent, not guaranteed Compile-time assert; panic if period > N
Max window length usize::MAX (practically unbounded) Chosen at build time (MAX_PERIOD = 1024)
Failure mode when full Reallocate + copy Programmer decides: Wrapping (overwrite) or Saturating (reject)

What this PR does

  • Replaces buf: VecDeque<f64> with
    buf: ArrayDeque<f64, MAX_PERIOD, Wrapping>.
  • Adds assert!(period ≤ MAX_PERIOD) to keep invariants honest.
  • Updates the invariant tests to assert buf.len() == count on every tick.
  • No public API changeSimpleMovingAverage stays the same struct/trait combo.

Trade-offs & future considerations

  • Hard ceiling: if a caller genuinely needs a larger period they must re-compile with a bigger MAX_PERIOD or make it a const-generic parameter.
  • Stack size: the SMA instance now carries MAX_PERIOD × f64 bytes on the stack (≈ 8 KiB with the current 1024 cap). For deeply nested indicators we may revisit this.
  • no_std: arraydeque already offers a #![no_std] build—handy for embedded users if we ever target bare-metal.

Key takeaway: VecDeque is dynamic and safe but surprises you with allocations; ArrayDeque is immovable and predictable—exactly what a moving-average window needs.


Why did the momentum tests (bias.rs, macd.rs) start red-lining?

Short answer: they were comparing bit-patterns, not behaviour.
The SMA refactor swaps re-summing the deque on every tick for a running sum that adds the new price and subtracts the price that just rolled out of the window. Same maths, but the order of floating-point operations changes, so the last ≈ 1 × 10⁻¹⁵ of the result can flip.

Before (VecDeque) After (ArrayDeque) Result in tests
sum = inputs.iter().sum()re-sums N numbers each tick. Stable reduction order. sum += price; sum -= oldest;incremental update. Different reduction order ⇒ different rounding noise. Mathematical value unchanged, but the IEEE-754 bit pattern no longer matches the hard-coded constants in the two tests.

crates/indicators/src/momentum/bias.rs

*Old test

 assert!(approx_equal(bias.value, 0.0006547359231776628));

with a default tolerance of ~1 × 10⁻¹⁴.

  • New SMA differs by ~6 × 10⁻¹⁶ (≈ 1 ULP).

  • Fix — keep the analytical target but make the bound explicit and self-documenting:

    const EPS: f64 = 1e-12;
    assert!(abs_diff_lt(bias.value, EXPECTED));

crates/indicators/src/momentum/macd.rs

  • Test did an assert_eq! on
    -2.500_000_000_016_378e-5.
  • Incremental update changed the low-order bits (Δ ≈ 1.6 × 10⁻¹⁷).
  • Fix — use the project-wide approx_equal helper with a 1 × 10⁻¹² ring-fence.

Why this is not hiding a logic bug

  • Analytical cross-check: running-sum and re-sum give identical results over ℝ; only FP rounding changed.
  • Property tests now ensure the running sum never drifts from the deque contents.
  • ULP-level deltas are inevitable whenever the reduction order changes; that is why numerical code must use tolerant comparisons.

Take-away for future tests

  1. Never use assert_eq!(f64, f64) unless checking for NAN, INFINITY, etc.
  2. Always compare within a documented tolerance that scales with the magnitude of the expected value.

@nicolad nicolad requested a review from cjdsellers May 24, 2025 20:06
@nicolad nicolad self-assigned this May 24, 2025
@nicolad nicolad added the rust Relating to the Rust core label May 24, 2025
@nicolad nicolad force-pushed the 2507-arraydeque branch from 4f8a7f7 to c00c9c6 Compare May 24, 2025 20:09
@nautechsystems nautechsystems deleted a comment from CLAassistant May 24, 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.

Thanks @nicolad 👌

  • The ArrayDeque is a more efficient structure for this
  • Good call on the use of approx_equal

@cjdsellers cjdsellers changed the title SMA: swap VecDeque for fixed-capacity ArrayDeque Change VecDeque for fixed-capacity ArrayDeque in SMA May 24, 2025
@cjdsellers cjdsellers merged commit 4181fc7 into develop May 24, 2025
17 checks passed
@cjdsellers cjdsellers deleted the 2507-arraydeque branch May 24, 2025 22:17
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