feat(filter): equinoctial GVE + element-space orbital filtering - #148
Conversation
…ent-space-orbital-filtering) Completes the element-space-orbital-filtering change (14/14 tasks) — the advanced-methods survey's space-accuracy centerpiece. All additive: existing models, trackers, and every calibrated benchmark unchanged. - thresh-core/orbital/gve: Gauss variational equations for the stored direct equinoctial set (a, h, k, p, q, lambda), transcribed from the fetched Stacey & D'Amico arXiv:2105.06516 Appendix A (naming map documented; the appendix's corrected Roth H-bar sign); perturbing acceleration as a time-aware inertial closure rotated into RSW internally (J2 enters as j2_acceleration alone; two-body secular analytic); sub-stepped RK4; near-singular (e,i ~ 1e-8) finite; cross-formulation equivalence vs the Cartesian two-body+J2 path in both directions across LEO/MEO/eccentric-through-perigee at measured sub-decimetre tolerances — the transcription validated by an independent in-tree formulation sharing only the force closure - thresh-filter/models/equinoctial: MotionModel over the 6D element state (KeplerJ2 conventions); mean longitude UNWRAPPED in filter state space with wrap-straddle and hundreds-of-radians proofs; elements->ECI position measurement mapping bitwise-equal to the existing conversion; process noise = isotropic acceleration PSD through the GVE input matrix (Stacey & D'Amico Eq. 5) — the same physical Q the Cartesian model encodes, so demonstration fairness is by construction - The measured payoff (thresh-eval/tests/orbital_coast_gap.rs, seeded, 50 runs, bitwise-deterministic): coasting a 7000 km LEO, the Cartesian EKF's position ANEES exits the 95% band once per revolution as the banana rotates (4.757 at 2.39 rev vs band [2.360, 3.716]; again 4.370 at 3.35 rev) while the equinoctial UKF stays inside at every one of nine swept gaps (2.58-2.91) — the element-space consistency claim, measured. Demonstration isolates coast-phase propagation via a shared warmup posterior (divergence + rationale recorded in design.md) - Benchmark invariance: all four calibrated scenarios digit-for-digit identical to the pre-change baseline Adversarially reviewed (4 lenses incl. a term-by-term re-fetch of the GVE source): 6 findings, 2 confirmed and fixed (the fetched paper is by Stacey & D'Amico, not "Sullivan" — the exact filled-from-memory citation fingerprint the provenance rule exists to catch; a 21x minute-conversion slip in design prose), 4 refuted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesEquinoctial orbital filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Filter as EquinoctialModel
participant GVE as propagate_equinoctial
participant Rates as equinoctial_element_rates
participant Conversion as equinoctial_position
Filter->>GVE: predict six equinoctial elements
GVE->>Rates: evaluate RK4 stage rates
Rates-->>GVE: return element derivatives
GVE-->>Filter: return propagated state
Filter->>Conversion: map elements to ECI position
Conversion-->>Filter: return position measurement
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/thresh-core/src/orbital/gve.rs`:
- Around line 689-706: Extend gve_vs_cartesian_gap while preserving the existing
element-born path, then add a Cartesian-born case that initializes r0 and v0
with keplerian_to_cartesian, converts that state to equinoctial elements for
propagate_equinoctial, and runs the same three-regime assertions. Ensure both
initialization paths execute and are compared against the Cartesian propagator.
- Around line 350-355: Update substep_count to validate or guard max_step_s
before division so zero or invalid step sizes cannot produce a saturated
usize::MAX count, and calculate the count from dt.abs() so negative intervals
receive the same sub-stepping as positive intervals. Preserve the minimum
one-step behavior for zero-duration propagation and the existing usize return
contract.
In `@crates/thresh-eval/tests/orbital_coast_gap.rs`:
- Around line 401-445: Extend the sweep in the crossover-checking test to retain
each equinoctial UKF verdict, then assert after the loop that all nine recorded
verdicts are Consistent. Also assert the contractual Cartesian crossing
checkpoint indices if those two crossings are expected, while preserving the
existing crossover value and bound assertions.
In `@crates/thresh-filter/src/models/equinoctial.rs`:
- Around line 226-236: Update Equinoctial process-noise computation around
process_noise and inertial_input_matrix so Γ is evaluated from the propagated
nominal state rather than the unchanged reference_elements on every call.
Advance or otherwise maintain the nominal reference across each interval before
constructing Q, while preserving the existing PSD scaling; add coverage
demonstrating Q changes as orbital longitude changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54d06e50-c39a-421e-a33b-b55c4cfa07c4
📒 Files selected for processing (7)
crates/thresh-core/src/orbital/gve.rscrates/thresh-core/src/orbital/mod.rscrates/thresh-eval/tests/orbital_coast_gap.rscrates/thresh-filter/src/models.rscrates/thresh-filter/src/models/equinoctial.rsopenspec/changes/element-space-orbital-filtering/design.mdopenspec/changes/element-space-orbital-filtering/tasks.md
| fn process_noise(&self, dt: f64) -> DMatrix<f64> { | ||
| // Eq. (5) of Stacey & D'Amico with Φ ≈ I over the interval and the | ||
| // isotropic inertial acceleration PSD Q̃ᴵ = σ_accel²·I₃ (their Eq. 3): | ||
| // Q ≈ Γᴵ·(σ²I₃)·Γᴵᵀ·Δt = σ²·(Γᴵ Γᴵᵀ)·Δt. | ||
| // Symmetric positive-semidefinite (Gram form; rank ≤ 3 — added to the | ||
| // full-rank predicted covariance, so the sum stays PD). Γᴵ is frozen | ||
| // at the reference elements (module docs). This is the same physical | ||
| // PSD KeplerJ2 assumes (its Q is Eq. 5's closed form in Cartesian). | ||
| let g = inertial_input_matrix(&self.reference_elements, self.gravity.mu); | ||
| let scale = self.sigma_accel * self.sigma_accel * dt; | ||
| (&g * g.transpose()) * scale |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Advance the process-noise linearization with the nominal state.
Every call evaluates Γ at the unchanged reference_elements, although Γ depends on orbital phase. The coast test consequently injects the same element-space covariance orientation for four revolutions—and supplies that reference from truth[0].
The cited SNC model treats this as a small-interval approximation; longer propagation requires evaluating the state and covariance contributions across subintervals. (arxiv.org) Make process noise state-aware, or advance a nominal reference before computing each interval’s Q, and add a test showing Q changes with longitude.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/thresh-filter/src/models/equinoctial.rs` around lines 226 - 236,
Update Equinoctial process-noise computation around process_noise and
inertial_input_matrix so Γ is evaluated from the propagated nominal state rather
than the unchanged reference_elements on every call. Advance or otherwise
maintain the nominal reference across each interval before constructing Q, while
preserving the existing PSD scaling; add coverage demonstrating Q changes as
orbital longitude changes.
There was a problem hiding this comment.
Fixed in 87b26a2 — EquinoctialModel::re_anchored(state) added and the demo re-anchors Γ at the current mean each coast step; the model docs state the small-interval approximation and the re-anchoring contract for long arcs. Demo numbers unchanged at recorded precision (Q is small; its orientation now honestly tracks phase).
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- current commit ---'
git rev-parse --short HEAD
printf '%s\n' '--- equinoctial model outline ---'
ast-grep outline crates/thresh-filter/src/models/equinoctial.rs --items all --type function,method --match 're_anchored|process_noise|inertial_input_matrix|test|noise'
printf '%s\n' '--- re-anchoring and process-noise references ---'
rg -n -C 5 're_anchored\s*\(|process_noise\s*\(|inertial_input_matrix\s*\(|reference_elements' \
crates/thresh-filter/src/models/equinoctial.rs crates/thresh-eval/tests/orbital_coast_gap.rs
printf '%s\n' '--- target implementation sections ---'
sed -n '80,270p' crates/thresh-filter/src/models/equinoctial.rs
printf '%s\n' '--- target test sections ---'
sed -n '270,525p' crates/thresh-filter/src/models/equinoctial.rs
printf '%s\n' '--- coast demo re-anchoring contexts ---'
rg -n -C 10 're_anchored\s*\(' crates/thresh-eval/tests/orbital_coast_gap.rsLength of output: 36698
@montge, the long-arc behavior is now correctly re-anchored: coast_run builds step_model from the current ukf.x before every coast-step prediction, and the API documentation makes the small-interval contract explicit.
One requested piece is still missing: the unit tests do not yet demonstrate that re_anchored causes process_noise(dt) to change with orbital longitude. Please add that focused regression test; then this thread can be resolved.
✏️ Learnings added
Learnt from: montge
Repo: Chaffgold/thresh PR: 148
File: crates/thresh-filter/src/models/equinoctial.rs:241-251
Timestamp: 2026-07-17T02:59:58.708Z
Learning: In `crates/thresh-filter/src/models/equinoctial.rs`, `EquinoctialModel::process_noise` linearizes the Gauss input matrix at `reference_elements`. For long arcs, callers should use `EquinoctialModel::re_anchored(¤t_filter_mean)` before each predict step so the element-space process-noise orientation follows orbital phase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
…weep asserts, phase-tracking Q - gve.rs: substep count uses |dt| (backward propagation splits correctly) and propagate_equinoctial validates max_step_s (finite, > 0) - gve.rs: genuinely Cartesian-born equivalence direction added — (r0, v0) from keplerian_to_cartesian, GVE elements born FROM that state via cartesian_to_keplerian, so no shared elements-first initialization can cancel a conversion defect; measured gap 0.138 m over 3 revs is initial-condition conditioning (near-circular round trip ~mm delta-a drifting 3*pi per rev), documented, tolerance 1.0 m vs km-scale for a real defect - orbital_coast_gap.rs: the full stated result is now asserted — the equinoctial UKF in-band at ALL nine gaps and both recorded Cartesian excursions (checkpoints 5 and 7), not just one crossover - EquinoctialModel: re_anchored(state) helper (Clone+Copy derived); the SNC input matrix Gamma depends on orbital phase, so the demo re-anchors per coast step and the model documents the small-interval approximation and the re-anchoring contract for long arcs; demo numbers unchanged at recorded precision Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
… completed change (#149) Implementation merged in #148 (14/14 tasks). Syncs the two delta specs into main specs — gauss-variational-equations and equinoctial-filter-model — then moves the change to openspec/changes/archive/2026-07-17-element-space-orbital-filtering. openspec validate --all --strict --no-interactive: 50/50 post-archive. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Summary
Implements
element-space-orbital-filtering(#147 spec; 14/14 tasks) — the advanced-methods survey's space-accuracy centerpiece. All additive: existing models, trackers, and every calibrated benchmark unchanged.thresh-core/orbital/gve — Gauss variational equations
(a, h, k, p, q, λ), transcribed from the fetched Stacey & D'Amico (arXiv:2105.06516) Appendix A — naming map documented, including the appendix's correction of a sign error in Roth's H̄j2_accelerationalone; two-body secular analytic) — any force the propagation tier's config can express can later drive element filteringthresh-filter/models/equinoctial — the filter model
MotionModelover the 6D element state; mean longitude unwrapped in filter state space (periodicity owned by conversions) with proofs at the wrap boundary and across hundreds of accumulated radians — the classic element-filter bug made structurally impossibleThe measured payoff
Seeded Monte-Carlo coast (7000 km / 51.6° LEO, 50 runs, bitwise-deterministic), position ANEES vs the 95% band [2.360, 3.716]:
The Cartesian banana pushes the EKF out of band once per revolution as its major axis rotates; the equinoctial filter stays inside at all nine swept gaps. The folklore claim, now a recorded number. (The demonstration isolates coast-phase propagation via a shared warmup posterior — rationale and the measured element-space measurement-update trade-off recorded in design.md; that update path is the tracker-consumption follow-up's problem.)
Verification
--features adsb, the feat(core): atmospheric measurement propagation — refraction + ionospheric delay biases with correction hooks #145 lesson), clippy-D warnings, fmt, rustdoc-Dwarnings, openspec 49/49🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation