Fix angle-covariance units in convert (degrees, not radians)#342
Merged
Conversation
convert() reports the angular orbital elements (inc, node, argPeri, and ma for KEP/BKEP) in degrees, but left their covariance in radians^2 -- the angle *values* were scaled radians->degrees while the covariance was not. The reported angular uncertainties were therefore too small by 180/pi (their variances by (180/pi)^2 ~ 3283x) and inconsistent with the state they accompany, affecting every degree-bearing output format: COM, BCOM, KEP, BKEP. The existing round-trip tests miss this because the inverse conversion reads the (radian-assuming) covariance back, so the inconsistency cancels and the Cartesian covariance returns unchanged. Fix both sides symmetrically: scale the covariance rows/columns of each degree element by 180/pi on output, and by pi/180 when reading a degree-format orbit back in, so the covariance is always expressed in the same units as the state while the conversion math stays in radians. Verified against a finite- difference Jacobian: every format now matches to ~1e-9 (KEP/BKEP were ~100% off, COM/BCOM ~0.8% off before), and BCART->KEP->BCART round-trips to ~6e-14. Add test_convert_covariance_angle_units: a forward finite-difference check (the round-trip tests cannot catch this); it fails by ~100% without the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Validated by a broader covariance audit. This fix was found by an independent finite-difference-vs-analytic sweep of convert's covariance, and the same method confirms it:
For context, the audit covered the whole covariance pipeline (fit 6×6, convert, predict obs_cov + time propagation, error ellipse, weight_data) against independent checks (finite-difference / Monte-Carlo / JPL Horizons). The recurring theme: these were all hidden because existing tests checked round-trip or shape, never the values against an independent method — which is exactly why the new test here is a forward FD check rather than another round-trip. |
The degree->radian input covariance scaling did an in-place float multiply on the cov columns. A CSV of all-zero placeholder covariances (e.g. holman.csv) is read with int columns, and numpy >= 2 forbids casting a float multiply result back into an int array (same_kind rule), so `layup predict` on such an input crashed on the 3.14 CI leg. Cast the covariance columns to float when copying. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Brings in #344 (percentile IOD prefilter) so the 609631 macOS real-data validation passes — that was this branch's only red CI leg. Clean merge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The radians<->degrees covariance scaling was duplicated as two opaque nested string-keyed loops (input side with pi/180, output side with 180/pi), each silently depending on element_order's index matching the cov_i_j row/col index — an unenforced invariant that, if it drifted, would mis-scale reported angular uncertainties (the exact bug class this covariance fix exists to address). - Extract _scale_degree_cov(arr, fmt, factor, mask=None): one documented place for the diag(s)*C*diag(s) congruence; both call sites collapse to a single line (input passes its per-format row mask; output scales the whole column to mirror the unmasked value conversion directly above it). - Document the element_order<->cov-index invariant at the definition. - Add test_element_order_matches_degree_columns to pin the degree_columns / element_order dicts together (the numerical basis match is already covered by test_convert_covariance_angle_units). Behavior unchanged: convert round-trips + the forward-FD covariance test pass (11/11 in test_convert.py). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kjnapier
approved these changes
Jun 22, 2026
kjnapier
left a comment
kjnapier
left a comment
Collaborator
There was a problem hiding this comment.
I think this looks okay. the _apply_convert function is making me dizzy to look at, but the results seem to check out
Collaborator
Author
|
Thank you!
…On Mon, Jun 22, 2026 at 4:30 PM Kevin Napier ***@***.***> wrote:
***@***.**** approved this pull request.
I think this looks okay. the _apply_convert function is making me dizzy to
look at, but the results seem to check out
—
Reply to this email directly, view it on GitHub
<#342?email_source=notifications&email_token=AA5CVWIVYHLOKD5VU25MDXT5BGJNDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJUG44DMNZYHE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#pullrequestreview-4547867895>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA5CVWJXPXLXCQZI337OKCL5BGJNDAVCNFSNUABFKJSXA33TNF2G64TZHM4TCNRSHAYTGMRUHNEXG43VMU5TINZRGAYTOMRVHE32C5QC>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/AA5CVWPN62LB2YVLUCIKPYD5BGJNDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJUG44DMNZYHE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KUZTPN52GK4S7NFXXG>
and Android
<https://github.com/notifications/mobile/android/AA5CVWJMQ7YEPI6DOBYFGU35BGJNDA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINJUG44DMNZYHE22M4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2K4ZTPN52GK4S7MFXGI4TPNFSA>.
Download it today!
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Matthew J. Holman, PhD
Senior Astrophysicist
Center for Astrophysics | Harvard & Smithsonian
60 Garden Street, MS #51
Cambridge, MA 02138
(617) 496-7775
|
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
2 participants
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
convert()reports the angular orbital elements (inc,node,argPeri, andmafor KEP/BKEP) in degrees, but left their covariance in radians² — the angle values were scaled radians→degrees while the covariance was not. So the reported angular uncertainties were too small by 180/π (variances by (180/π)² ≈ 3283×) and inconsistent with the state they accompany. This affects every degree-bearing output format: COM, BCOM, KEP, BKEP — i.e. the human-readable orbit outputs.Why it wasn't caught
The existing round-trip tests (
BCART → format → BCART) miss it: the inverse conversion reads the covariance back assuming radians, so the inconsistency cancels and the Cartesian covariance returns unchanged (~6e-14). The bug only shows up if you look at the reported covariance of a degree-format orbit, which no test did.Found via a finite-difference audit of convert's covariance across formats and regimes:
Fix
Scale the covariance rows/columns of each degree element symmetrically: by 180/π on output, and by π/180 when reading a degree-format orbit back in, so the covariance is always in the same units as the state while the conversion math stays in radians. Round-trips remain exact (
~6e-14).Test
test_convert_covariance_angle_units— a forward finite-difference check of the reported covariance against the conversion Jacobian (the round-trip tests structurally cannot catch this). Fails by ~100% without the fix; passes at ~1e-9 with it. Full convert suite: 10 passed.🤖 Generated with Claude Code