Fix #286: handle observatories with zero-valued parallax constants - #339
Merged
Conversation
LayupObservatory.obscodes_to_barycentric crashed with "Observatory ...
has invalid coordinates" for several real observatory codes, including the
common geocentric code 500. The root cause is in Sorcha's
Observatory.convert_to_geocentric, which gates on the truthiness of the
parallax constants (obs_location.get("sin", False)). A constant that is
legitimately 0.0 -- the geocenter (500/244/248: Longitude=cos=sin=0),
Greenwich (000: Longitude=0), or an equatorial station such as Quito
(782: sin=0) -- is falsy, so the observatory is wrongly reported as having
no fixed position. Layup then routes it to the moving-observatory path,
which raises for plain MPC input that carries no per-observation position.
Override convert_to_geocentric in LayupObservatory to test for the presence
of the constants (is not None) rather than their truthiness. Codes with no
position keys at all (roving observer 247, space telescopes WISE/TESS/HST)
still return (None, None, None) and correctly take the ADES-position path;
the geocenter resolves to a (0, 0, 0) offset from Earth's center, which is
exactly right. Add a regression test that fails without the fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matthewholman
added a commit
that referenced
this pull request
Jun 22, 2026
Brings the branch up to date with main (36 commits: the multi-root IOD picker + residual prefilter, IAS15 adaptive_mode=2, #339-#344 fixes, docs/dash_ui). Resolves the one conflict in src/layup/orbitfit.py, where bk-everywhere had added the engine= dispatch to the pre-picker do_fit while main rewrote do_fit around the multi-root picker. Resolution keeps main's picker/prefilter pipeline and threads `engine` through it: - _run_fit() gains iter_max; it is forwarded to the Cartesian engine (run_from_vector_with_initial_guess, which main parameterized) and ignored by bk_native (run_bk_native_fit takes mu and uses its own internal LM cap). - do_fit() keeps all of main's picker knobs and adds engine="cartesian". - the two picker LM passes and the incremental-fit loop now dispatch through _run_fit(..., engine, iter_max). Validated in an isolated build: 54/54 tests pass across test_bk_basis, test_bk_fit, test_bk_everywhere, and test_orbit_fit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kjnapier
approved these changes
Jun 23, 2026
kjnapier
left a comment
kjnapier
left a comment
Collaborator
There was a problem hiding this comment.
The new gate logic looks good to me. I did not check the correctness of the old expressions, but assuming they did not change, all is good.
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
Fixes #286.
layup orbitfitcrashed withValueError: Observatory <code> has invalid coordinatesfor several real observatory codes — most importantly the common geocentric code 500, plus 000 (Greenwich), 782 (Quito), and 244/248.Root cause
The crash originates in Sorcha's
Observatory.convert_to_geocentric(simulation_parsing.py), which gates on the truthiness of the parallax constants:A parallax constant that is legitimately
0.0is falsy, so the observatory is wrongly reported as having no fixed position. Layup then routes it to the moving-observatory path, which demands per-observationpos1/2/3fields and raises for plain MPC input that carries none. The affected codes all have a constant that is exactly0.0:Fix
Override
convert_to_geocentricinLayupObservatoryto test for the presence of the constants (is not None) rather than their truthiness. Sincesuper().__init__callsself.convert_to_geocentric, the override is used when the observatory table is built — no upstream release needed.(None, None, None)and correctly take the ADES per-observation position path.(0, 0, 0)offset from Earth's center, which is exactly right.The same bug should also be fixed upstream in Sorcha (a separate one-line PR); this override makes layup correct immediately and is robust regardless of the installed Sorcha version.
Verification
test_fixed_observatory_with_zero_parallax_constant— passes with the fix, fails without it.tests/layup/test_data_processing_utilities.py: 40 passed.500observation now flows throughobscodes_to_barycentricand resolves to Earth's barycentric position; Greenwich000lands exactly one Earth radius (4.25e-5 AU) from the geocenter.🤖 Generated with Claude Code