Make predict_sequence march (integrate once, interpolate per detection) - #428
Merged
Conversation
predict_sequence looped detections calling predict_from_fit_result -> predict, which does reb_simulation_create ... reb_simulation_free PER detection, so an orbit was re-integrated from its epoch for every requested time -- N detections = N full integrations. residuals_at_state (#406) already solved this for the fit path with a sorted forward+backward single pass; predict_sequence just never got the same treatment. Rework predict_sequence to mirror residuals_at_state: split detections into those after the epoch (marched forward) and before it (marched backward), each sorted so one sim integrates the trajectory once per direction and integrate_light_time -> assist_integrate_or_interpolate interpolates at each detection. Each detection keeps its own observer position, so a single call may now mix observatories. Refactored the per-detection geometry+covariance out of predict() into a reusable compute_single_predict(ax, var, ...) that operates on an existing sim; the bound single predict() keeps its own sim and behavior. Because integrate_or_interpolate makes the integration independent of the query times, results are BIT-IDENTICAL to the per-detection path (verified: 0 difference in RA/Dec/ellipse), at a fraction of the integrations. Adds test_predict_sequence_marches_equivalently (one multi-time call == per-time calls). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test_bench_residuals_runs asserted residuals_at_state is faster than predict_sequence (speedup > 1), which held only because predict_sequence re-integrated from the epoch per detection. Now that predict_sequence uses the same sorted single-pass march, the two are comparable (ratio ~1, e.g. 0.998 on CI), so that assertion is no longer valid. Relax it to guard only against a pathological regression (> 0.5, i.e. not more than ~2x slower) -- consistent with this file's 'not a performance assertion' intent -- and update the benchmark's docstring/framing to reflect that both paths march. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matthewholman
added a commit
that referenced
this pull request
Jul 7, 2026
Resolve test_predict.py conflict with #428 (predict_sequence march): keep both new tests (mixed/single obscode + marching equivalence) as separate self-contained functions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
predict_sequencere-integrated each orbit from its epoch for every requested time. It looped detections callingpredict_from_fit_result→predict, andpredictdoesreb_simulation_create … assist_free … reb_simulation_freeper call — so predicting an orbit to N times ran N full integrations from the epoch, each with its own IAS15 ramp-up.residuals_at_state(#406) already solved exactly this for the fit/rejection path with a sorted forward + backward single pass that reuses one sim and letsintegrate_or_interpolateinterpolate at each detection.predict_sequencesimply never got the same treatment. This PR gives it that treatment.Change
predict_sequencenow marches. It splits detections into those after the epoch (marched forward) and before it (marched backward), each sorted, and runs each pass against a single sim — so the trajectory is integrated once per direction andintegrate_light_time → assist_integrate_or_interpolateinterpolates at each detection, instead of a fresh integration per detection. Mirrorsresiduals_at_state/create_sequences.predict()into a reusablecompute_single_predict(ephem, ax, var, …)that operates on an existing sim. The bound single-observationpredict()keeps its own sim create/free and is behavior-unchanged.predict_sequencecall may now mix observatories (each detection is light-corrected against its own station).Correctness
integrate_or_interpolatemakes the integration independent of the query times (it interpolates at each), so the marching result is bit-identical to the per-detection path:(verified across times both before and after the epoch, i.e. both passes). New test
test_predict_sequence_marches_equivalentlyasserts a one-call multi-time prediction equals per-time predictions to integrator precision.Impact
Any multi-time prediction benefits — ephemeris generation, the
predictCLI over a time grid, and covariance-aware prediction of a catalog to many epochs — turning N integrations per orbit into ~1. No API change (predict/predict_sequencesignatures and outputs are unchanged).🤖 Generated with Claude Code