Skip to content

Fix Obs80 reader emitting positionless orphan observations (deleted obs + S/s mis-pairing) - #425

Merged
matthewholman merged 4 commits into
mainfrom
fix/obs80-satellite-pairing
Jul 6, 2026
Merged

Fix Obs80 reader emitting positionless orphan observations (deleted obs + S/s mis-pairing)#425
matthewholman merged 4 commits into
mainfrom
fix/obs80-satellite-pairing

Conversation

@matthewholman

@matthewholman matthewholman commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Summary

The obs80 reader let two kinds of malformed / non-observation lines reach the fitter as positionless records (sys='', pos=nan). For a space-based obscode (e.g. WISE / C51) such a row carries no ADES observer position and, downstream, falls back to a per-row JPL Horizons lookup — which 503-throttled under the high parallelism of the full MPC-catalog fit (surfaced on object j4767).

Two related bugs, both fixed here:

  1. Deleted / replaced observations (MPC note 2 code X / x) were read as normal observations. j4767's WISE block contains one x line at the same timestamp as a kept satellite obs (a superseded measurement of the same exposure); tests/data/03666.txt contains a 1938 X photographic plate. These are now skipped.

  2. Two-line records were paired purely by position. Satellite S / radar R / roving V first lines were merged with whatever line followed, with no check that the follower was the matching lower-case s / r / v continuation. A desynchronised file (a duplicated or orphaned continuation line, or a first line whose continuation is missing) could mis-pair or emit a positionless standalone.

Change

The three previously independent pairing loops (get_row_count, read_rows, read_objects / _build_id_map) are consolidated into a single _iter_records generator that is now the sole source of truth for how raw lines group into records. It:

  • skips deleted observations (X / x),
  • validates each continuation line against its first line (lower-case note 2 + matching designation cols 1–14 + matching obscode cols 78–80),
  • drops orphan / mismatched continuations and continuation-less first lines instead of mis-pairing,

so every read path agrees on the record set (len(read_rows()) == get_row_count(), and read_objects stays index-aligned with _build_id_map).

Validation

Verified on the real j4767 bytes from the catalog run:

reader rows C51 rows C51 positionless
before 452 13 1 (→ Horizons)
after 451 12 0 (all ICRF_KM)

Tests added:

  • a real-data fixture (tests/data/j4767_wise_excerpt.txt, j4767's WISE/C51 block) asserting all 12 satellite records carry their ICRF_KM position and the deleted x line is dropped;
  • synthetic deleted, duplicate-continuation, leading-orphan, missing-continuation, and count/read/objects-consistency cases (all four desync cases fail on the old reader, pass on the new).

All pre-existing Obs80Reader tests pass. tests/data/03666.txt's expected row count is updated 4313 → 4312 — the old value had counted the deleted 1938 plate as a real observation.

🤖 Generated with Claude Code


Update — subsumes #411

Folded in #411 (issue/402-407 Obs80Reader robustness), now closed as superseded. This PR keeps #411's convert_obs80 by-type two-line dispatchS satellite ICRF, V roving-observer WGS84 geodetic (previously mis-read as a satellite position, #402/#282), R clear 'ingest via ADES' error — plus its _is_header_row/#407 blank-line guards and its roving/radar/blank-line tests + obs80_two_line_records.txt fixture. Combined with the deleted-X/x skip and S/s orphan fix here, _iter_records is now the single source of truth for record grouping across all read paths. Closes #402, #407. 18 Obs80Reader tests pass.

Closes #402
Closes #407

matthewholman and others added 2 commits July 6, 2026 06:55
Two related obs80 parsing bugs let malformed/non-observation lines reach the
fitter as positionless records (sys='', pos=nan). For a space-based obscode
(e.g. WISE / C51) such a row has no ADES observer position and falls back to a
per-row JPL Horizons lookup -- which 503-throttled under the high parallelism
of the full MPC-catalog fit (surfaced on object j4767).

1. Deleted/replaced observations (MPC note 2 code X / x) were read as normal
   observations. j4767's WISE block contains one such 'x' line at the same
   timestamp as a kept satellite obs; 03666.txt contains a 1938 'X' plate.
   These are now skipped.

2. Two-line records (satellite S / radar R / roving V + their lower-case
   s/r/v continuation) were paired purely by position, with no check that the
   follower was actually the matching continuation. A desynchronised file (a
   duplicated or orphaned continuation line, or a first line whose continuation
   is missing) would mis-pair or emit a positionless standalone.

Consolidate the three ad-hoc pairing loops (get_row_count, read_rows,
read_objects/_build_id_map) into one _iter_records generator that skips deleted
observations, validates each continuation against its first line (lower-case
note2 + matching designation + matching obscode), and drops orphans/mismatches
instead of mis-pairing -- so every read path agrees on the record set.

Tests: real-data fixture from j4767's WISE/C51 block (12 satellite records, all
with ICRF_KM positions, deleted 'x' line dropped) plus synthetic deleted,
duplicate-continuation, leading-orphan, missing-continuation, and
count/read/objects-consistency cases. 03666.txt row count is 4312 (was 4313,
which had counted the deleted 1938 plate).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_iter_records had no length check, so a trailing blank or truncated line fell
through to the single-line branch and reached convert_obs80, which raises on the
missing columns. main never carried the issue-#407 guard, and the numbered
catalog run relied on it over the same bulk MPC data. Skip any line shorter than
the note-2 column (15 chars) up front. Surfaced by the USDF run/catalog-build
cross-check of this branch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@matthewholman
matthewholman requested a review from kjnapier July 6, 2026 12:00
Reconcile with PR #411 (issue/402-407 Obs80Reader robustness), which overlaps
this branch's Obs80Reader rewrite. #411's structural pairing is superseded by the
_iter_records consolidation here, but its convert_obs80 by-type dispatch is
complementary and kept:

- S -> satellite geocentric ICRF position (km/AU), as before;
- V -> roving observer, parsed as WGS84 geodetic lon/lat (deg) + altitude (m)
  instead of mis-read as a satellite position (issue #402 / #282);
- R -> radar, raise a clear 'ingest via ADES' error instead of mis-parsing.

Also adds #411's _is_header_row length guard. Brings over #411's tests (roving,
radar, blank-line mixed file) and its obs80_two_line_records.txt fixture. With
this, #411 can be closed as superseded. 18 Obs80Reader tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@kjnapier kjnapier left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@matthewholman
matthewholman merged commit 054ca53 into main Jul 6, 2026
7 checks passed
@matthewholman
matthewholman deleted the fix/obs80-satellite-pairing branch July 6, 2026 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants