Fix weight_data=True: NameError + arcsec/radian unit bug#329
Open
matthewholman wants to merge 1 commit into
Open
Fix weight_data=True: NameError + arcsec/radian unit bug#329matthewholman wants to merge 1 commit into
matthewholman wants to merge 1 commit into
Conversation
The weight_data=True code path has been broken since it was added.
Two distinct bugs, hit in sequence:
1. NameError on `astcat_column_present`.
`_orbitfit` defined the flag as `g_column_present = "astCat" in
column_names` but then referenced `astcat_column_present` two
lines later inside the debias and weighting branches. The path
was reached the moment `weight_data=True` was passed, so the
feature crashed before doing any fitting. Fix: rename the
definition to match the usage sites (`astcat_column_present`).
2. Arcsecond vs radian unit mismatch.
`data_weight_Veres2017` returns the astrometric uncertainty in
ARCSECONDS (per its docstring). The call site assigned the
return value directly to `Observation.ra_unc` and `dec_unc`,
which are stored in RADIANS. That made each observation's
weight ~200000^2 = 4e10x too loose, inflating the fit covariance
to absurd values (sigma_pos ~ 25 million km on a 3 AU object) and
driving chi-square to zero. Fix: convert arcsec -> radian at
the assignment.
Effect on layup's reported position uncertainty for the three real
MPC objects in the test fixture (mainbelt asteroids, ~2-3 AU):
Object weight_data=False weight_data=True (after fix)
------ ----------------- ----------------------------
119839 269 km 123 km
742428 340 km 160 km
609631 306 km 80 km
With the fix, weight_data=True roughly halves the reported
sigma_pos (closer to JPL Horizons' published per-element sigmas
for these objects), and chi-square/dof moves from artificially
low (0.05-0.23, suggesting the default 1-arcsec uncertainty
overweights the data) to a more honest 0.27-0.53.
tests/layup/test_weight_data.py adds two regression tests:
- test_orbitfit_weight_data_true_runs_without_crashing:
catches the NameError-type regression on the wd=True path.
- test_orbitfit_weight_data_true_gives_sensible_uncertainty:
asserts sigma_pos < 10000 km for the 27-year mainbelt arc --
well above the actual ~120 km but trivially below the
25-million-km failure mode of the units bug.
Co-Authored-By: Claude Opus 4.7 (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.
This branch was developed with Claude Code, on top of an initial
implementation I had started.
Fixes two distinct bugs that have made
orbitfit(weight_data=True)non-functional since the feature was added. Either bug alone is
sufficient to break the path; both must be fixed for it to do
anything sensible.
Bug 1:
NameError: name 'astcat_column_present' is not defined_orbitfitinsrc/layup/orbitfit.pydefined the flag asbut then referenced
astcat_column_presenttwo lines later insidethe debias and weighting branches. As soon as
weight_data=True(or debias) was passed, this raised
NameErrorbefore any fittingcould happen.
Fix: rename the definition to match the usage sites
(
astcat_column_present). One-line change.Bug 2: arcsecond vs radian unit mismatch
data_weight_Veres2017returns the astrometric uncertainty inarcseconds (per its docstring). The call site assigned the
return value directly to
Observation.ra_uncanddec_unc, whichare stored in radians. Each observation's weight was therefore
~(180·3600/π)² ≈ 4×10¹⁰× too loose -- inflating the fit covariance
to absurd values (sigma_pos ~25 million km on a 3 AU object) and
driving chi-square/dof to zero (residuals dwarfed by the assumed
noise floor).
Fix: convert arcsec → radian at the assignment, with a comment
explaining why.
Effect
Layup's reported position uncertainty for three real MPC objects
(mainbelt asteroids at ~2-3 AU, from the existing test fixture):
weight_data=Falseweight_data=True(after fix)With the fix:
sigmas for these objects)
the default 1-arcsec uncertainty over-weights the data) to a more
honest 0.27-0.53
Tests
tests/layup/test_weight_data.py(2 tests):test_orbitfit_weight_data_true_runs_without_crashing:guards against the NameError-class regression -- exercises the
astcat_column_presentbranch by callingorbitfit(..., weight_data=True)on real MPC data with noastCatcolumn.test_orbitfit_weight_data_true_gives_sensible_uncertainty:asserts
sigma_pos < 10000 kmfor the 27-year mainbelt arc.Well above the actual ~123 km, but trivially below the
25-million-km failure mode of the units bug.
Dependencies
Independent of all other open PRs -- branches off
main, touchesonly
src/layup/orbitfit.pyand adds one test file.Review Checklist for Source Code Changes
tests/layup/test_weight_data.py4_random_mpc_ADES_provIDs_no_sats.csv)