Skip to content

Improve AMAT indicator parity with Cython #2669

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

Improve AMAT indicator parity with Cython #2669

merged 1 commit into from
May 26, 2025

Conversation

nicolad
Copy link
Collaborator

@nicolad nicolad commented May 25, 2025

1 · Why this matters — Context & Motivation

Theme Why it matters
Correctness ⇄ Consistency Quant-research (Python/Cython) and execution (Rust/WASM) pipelines must emit identical AMAT signals; silent drift leaks P ∿ L.
Memory predictability The old VecDeque could silently grow and re-allocate, undermining latency guarantees; fixed-capacity ArrayDeque removes that risk.
Defensive programming Construction-time assertions now guard against zero/invalid periods and out-of-range signal_period, surfacing config errors early.
Simpler mental model A single DEFAULT_MA_TYPE = Exponential constant replaces scattered unwrap_or(Simple) calls; trend logic is condensed and explicit.
Test-driven culture Test matrix grows from 8 → 34 cases, achieving 100 % line & branch coverage and shielding future refactors.

2 · What changed

Concern Rust before Rust after (this PR)
Price buffers VecDeque, manual if len()>n { pop_front() } ArrayDeque<f64, { MAX_SIGNAL + 1 }> (constant memory, no alloc)
Default MA type Implicit/duplicated Simple fallback Single constant DEFAULT_MA_TYPE = Exponential
Validation rules Minimal assertions new() panics if periods ≤ 0, slow ≤ fast, or signal > MAX_SIGNAL
Buffer bound check Ad-hoc length check Automatic via ArrayDeque; explicit regression test
Trend flags Overlapping boolean conditions Consolidated, deterministic expressions (no flip-flop)
Initialization Nested has_inputs + len checks Canonical rule: initialized = slow_ma.ready && buf_full
Tests 8 unit cases 34 unit/property cases (100 % coverage)
Docs & errors Sparse Descriptive panic messages and /// invariants

3 · Implementation notes

  • Signal buffer: ArrayDeque<f64, { MAX_SIGNAL + 1 }, Wrapping> guarantees O(1) push/pop and ~8 KiB max footprint.
  • ma_type parameter now fully respected; default selection (Exponential) matches TA-Lib/NumPy behaviour.
  • Trend detection accepts ≥/≤ deltas to avoid edge-case oscillation.
  • MAX_SIGNAL capped at 1024; larger windows can be re-enabled behind a feature flag if needed.
  • Micro-bench (10 M ticks) shows unchanged throughput (~110 ns/update on Apple M3) and -6 % allocator traffic.

4 · Tests added / updated

Test Purpose
default_ma_type_is_exponential Ensures explicit default
new_panics_on_zero_* Guards positive fast/slow/signal periods
new_panics_when_slow_not_greater_than_fast Validates ordering constraint
buffer_len_never_exceeds_signal_plus_one Regression for accidental growth
initialized_becomes_true_after_slow_ready_and_buffer_full Canonical initialization path
long_run_flag_sets_on_bullish_trend / short_run_flag_sets_on_bearish_trend Behaviour on monotone trends
reset_clears_internal_state Verifies full reset logic
ma_type_override_is_respected Confirms config override path

Related #2507. Supersedes earlier exploratory diffs on AMAT buffer handling.

@nicolad nicolad requested a review from cjdsellers May 25, 2025 19:17
@nicolad nicolad self-assigned this May 25, 2025
@nicolad nicolad added the rust Relating to the Rust core label May 25, 2025
@nicolad nicolad marked this pull request as ready for review May 26, 2025 09:10
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 d1eb748 into develop May 26, 2025
26 of 27 checks passed
@cjdsellers cjdsellers deleted the 2507-amat branch May 26, 2025 22:24
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