Obs80Reader: dispatch two-line records by type + guard blank lines (Closes #402, #407) - #411
Closed
matthewholman wants to merge 1 commit into
Closed
Obs80Reader: dispatch two-line records by type + guard blank lines (Closes #402, #407)#411matthewholman wants to merge 1 commit into
matthewholman wants to merge 1 commit into
Conversation
, #407) #402 -- roving observer records: the reader sent every two-line record (S/R/V) through the *satellite* parser, but a roving-observer ``v`` line carries geodetic longitude/latitude (deg) + altitude (m) on WGS84, not a geocentric position, so it mis-parsed and crashed. Dispatch on the first line's note2: * S -> geocentric ICRF position (km/AU), as before; * V -> geodetic WGS84 longitude/latitude/altitude (the geodetic->geocentric-ICRF conversion belongs to the observatory, issue #282); * R -> radar; raise a clear error (radar is ingested via ADES delay/doppler). #407 -- blank/short lines: ``two_line_row_start`` and ``_is_header_row`` indexed columns without a length check, so a blank or truncated line raised IndexError and aborted the whole read. Guard both, and skip blank/short lines in the read loop. Adds a two-line-records fixture (single optical + satellite + roving + blank lines) and tests: roving parses to WGS84 lon/lat/alt, radar raises, blank lines are skipped, and satellite parsing is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
matthewholman
added a commit
that referenced
this pull request
Jul 6, 2026
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>
Collaborator
Author
|
Superseded by #425. That PR consolidates all three obs80 read paths ( |
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
1 participant
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
Two
Obs80DataReaderrobustness bugs the MPC full-catalogue fitting surfaced.#402 — roving-observer (
V/v) recordsThe reader routed every two-line record (
S/R/V) through the satellite parser. But a roving-observervline carries geodetic longitude/latitude (deg) + altitude (m) on WGS84, not a geocentric position — so it mis-read the fields and crashed. Now it dispatches on the first line's note2:S→ geocentric ICRF position (km/AU), unchangedV→ geodetic WGS84 longitude / latitude / altitude. (Capturing the position + its frame here; the geodetic→geocentric-ICRF conversion belongs to the observatory — Implement Additional ADES Observatory Formats as Needed #282, whose WGS84 half this feeds.)R→ radar; raise a clear error (radar is ingested via ADES delay/doppler, not the obs80 reader)#407 — blank / short lines
two_line_row_startand_is_header_rowindexed columns (line[14],line[3]) with no length check, so a blank or truncated line raisedIndexErrorand aborted the entire read. Both are now length-guarded, and the read loop skips blank/short lines.Tests
New fixture
tests/data/obs80_two_line_records.txt(single optical + satellite + roving + interspersed blank lines) plus tests:WGS84with the correct lon/lat/altAll 10
test_Obs80Readertests pass; validated end-to-end on real (433) satellite+roving data.Closes #402. Closes #407.
🤖 Generated with Claude Code