Fix two bugs in Targeter finite-difference corrector#526
Open
KonradStanski wants to merge 1 commit intonyx-space:masterfrom
Open
Fix two bugs in Targeter finite-difference corrector#526KonradStanski wants to merge 1 commit intonyx-space:masterfrom
KonradStanski wants to merge 1 commit intonyx-space:masterfrom
Conversation
1. Remove spurious .transpose() on VNC→inertial DCM in convergence path. The convergence branch (building corrected_state from xi_start + total_correction) transposed the dcm_to_inertial rotation matrix, effectively applying the Δv in the wrong direction. All three other call sites in the same file use the DCM without transpose. This caused apply_with_traj() to fail verification on any VNC-frame targeting. 2. Move init_guess application outside the per-variable loop. Previously, state_correction was applied to xi inside the loop over variables, causing cumulative over-application: with 3 velocity variables [vx, vy, vz], the first variable's guess was applied 3 times, the second twice, and the third once. The corrected_state reconstruction (which applies total_correction once) would then disagree with the iterated xi, causing verification failures on sensitive arcs even in inertial frame. Now matches the pattern used in the iteration loop, which correctly accumulates then applies once. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the initial guess application in the Raphson finite difference targeter by accumulating state corrections and applying them once after the loop, and corrects a matrix calculation by removing an unnecessary transposition. Feedback indicates that the new !finite_burn_target check should be removed to ensure consistency with the main iteration loop, allowing impulsive state corrections to be applied even when finite burn variables are present.
ChristopherRabotin
approved these changes
Apr 19, 2026
|
|
||
| // Apply the initial guess | ||
| // Apply the initial guess: first accumulate, then apply once (matching | ||
| // the pattern used in the iteration loop at lines ~608-729). |
Member
There was a problem hiding this comment.
Suggested change
| // the pattern used in the iteration loop at lines ~608-729). | |
| // the pattern used in the iteration loop). |
I prefer not referring specific line numbers in comments.
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
.transpose()on VNC→inertial DCM in the convergence path ofraphson_finite_diff.rs, which causedapply_with_traj()to fail verification on any VNC-frame targetingDetails
Bug 1 — DCM transpose: The convergence branch (building
corrected_statefromxi_start + total_correction) transposed thedcm_to_inertialrotation matrix at line ~549, effectively rotating inertial→VNC instead of VNC→inertial. All three other call sites in the same file (init guess application, Jacobian perturbation, iteration step) use the DCM without transpose.Bug 2 — Init guess double-counting:
state_correctionwas applied toxiinside thefor (i, var)loop. With 3 velocity variables[vx, vy, vz]:xi.vel += [vx, 0, 0]xi.vel += [vx, vy, 0](accumulated) → total[2·vx, vy, 0]xi.vel += [vx, vy, vz]→ total[3·vx, 2·vy, vz]But
total_correctioncorrectly stores[vx, vy, vz], so the convergence path'scorrected_statedisagrees with the iteratedxi. The fix moves application outside the loop, matching the pattern already used in the iteration loop (~lines 608-729).Neither bug manifests when
init_guess = 0(the default fromVary::into()), which is why existing tests pass. Both bugs trigger on sensitive, long-arc targeting with nonzero initial guesses (e.g., trans-lunar injection).Test plan
cargo test --libtests passapply_with_trajverification passingCloses #525
🤖 Generated with Claude Code