From 5500dc43fdb5e7adc98418668c5d1032fb82df93 Mon Sep 17 00:00:00 2001 From: Matthew Holman Date: Sun, 5 Jul 2026 22:23:08 -0400 Subject: [PATCH 1/3] Fix Obs80 reader emitting positionless orphan observations 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) --- src/layup/utilities/file_io/Obs80Reader.py | 183 +++++++++++++-------- tests/data/j4767_wise_excerpt.txt | 25 +++ tests/layup/test_Obs80Reader.py | 114 ++++++++++++- 3 files changed, 248 insertions(+), 74 deletions(-) create mode 100644 tests/data/j4767_wise_excerpt.txt diff --git a/src/layup/utilities/file_io/Obs80Reader.py b/src/layup/utilities/file_io/Obs80Reader.py index 188a51f6..b5908884 100644 --- a/src/layup/utilities/file_io/Obs80Reader.py +++ b/src/layup/utilities/file_io/Obs80Reader.py @@ -3,16 +3,60 @@ from layup.utilities.file_io.ObjectDataReader import ObjectDataReader +# Column 15 (0-indexed 14) is the MPC "note 2" / observation-type code. The +# codes S (satellite), R (radar) and V (roving observer) each emit a SECOND +# line carrying the observer position/data; that continuation line repeats the +# designation and carries the same code in lower case (s / r / v). +_TWO_LINE_FIRST_NOTES = ("S", "R", "V") +_TWO_LINE_CONT_NOTES = ("s", "r", "v") +# Note 2 codes X / x flag an observation the MPC has deleted or replaced. Such a +# line is not a usable observation and must not be read as one -- in particular a +# deleted satellite (C-code) astrometry line carries no observer position, so +# emitting it produces a positionless record that would fall back to a per-row +# JPL Horizons lookup during fitting. +_DELETED_NOTES = ("X", "x") + + +def deleted_observation(line): + """Checks if the MPC Obs80 line is a deleted/replaced observation (note 2 + code X or x), which should be skipped entirely.""" + return len(line) > 14 and line[14] in _DELETED_NOTES + + def two_line_row_start(line): """Checks if the MPC Obs80 line is the first line of a two-line row format. - Column 15 (0-indexed 14) is the MPC "note 2" / observation-type code. The - codes S (satellite), R (radar) and V (roving observer) each emit a second - line carrying the observer position/data, so a line bearing one of them is - the first of a two-line record. + A line bearing an upper-case S, R or V in note 2 (column 15) is the first + of a two-line record. + """ + return len(line) > 14 and line[14] in _TWO_LINE_FIRST_NOTES + + +def two_line_row_continuation(line): + """Checks if the MPC Obs80 line is the second (continuation) line of a + two-line record. + + The continuation line repeats note 2 in lower case (s / r / v) and carries + the observer position rather than an astrometric measurement. It must never + be emitted as a standalone observation: on its own its position columns + would be misread as RA/Dec and it would carry no observatory position. """ - note2 = line[14] - return note2 == "S" or note2 == "R" or note2 == "V" + return len(line) > 14 and line[14] in _TWO_LINE_CONT_NOTES + + +def two_line_rows_match(first_line, second_line): + """Checks that ``second_line`` is the continuation line belonging to + ``first_line``: it is a continuation line, repeats the same designation + (columns 1-14) and reports the same observatory code (columns 78-80). + + This guards against a desynchronised file (an orphan continuation line, or + a first line whose continuation is missing) silently mis-pairing. + """ + if not two_line_row_continuation(second_line): + return False + if first_line[0:14] != second_line[0:14]: + return False + return first_line[77:80] == second_line[77:80] def ra_to_deg_ra(ra): @@ -174,6 +218,55 @@ def get_reader_info(self): """ return f"Obs80DataReader:{self.filename}" + def _iter_records(self, f): + """Yield one ``(main_line, second_line)`` tuple per logical obs80 record. + + This is the single source of truth for how the raw lines of the file + group into records; every read path (``get_row_count``, ``read_rows``, + ``read_objects``) walks it so their record counts and ordering always + agree. It pairs each two-line record (S/R/V first line + its lower-case + s/r/v continuation line) and, crucially, refuses to emit a malformed + observation: + + * a deleted/replaced observation (note 2 code X / x) is skipped; + * an orphan continuation line (one with no matching preceding first + line) is skipped -- it carries no astrometry of its own and would + otherwise be emitted as a positionless observation whose position + columns are misread as RA/Dec; + * a first line whose continuation is missing is dropped rather than + paired with the next unrelated line. + + ``main_line`` is the astrometry line; ``second_line`` is the + observer-position line, or ``None`` for a single-line observation. + """ + prev_first = None + check_header = True + for line in f: + if check_header and self._is_header_row(line): + continue + check_header = False + if deleted_observation(line): + # A deleted/replaced observation. If it was the first line of a + # two-line record its continuation is now orphaned and will be + # skipped by the branch below. + prev_first = None + continue + if two_line_row_start(line): + # Start of a two-line record. Any unconsumed previous first line + # had no continuation and is dropped. + prev_first = line + continue + if two_line_row_continuation(line): + if prev_first is not None and two_line_rows_match(prev_first, line): + yield prev_first, line + # else: orphan / mismatched continuation -> skip (emit nothing). + prev_first = None + continue + # A normal single-line observation. Any unconsumed previous first + # line lacked its continuation and is dropped. + prev_first = None + yield line, None + def get_row_count(self): """Return the total number of rows in the file. @@ -187,10 +280,8 @@ def get_row_count(self): """ row_cnt = 0 with open(self.filename, "r") as f: - for line in f: - # Skip empty lines, header rows, and the starting line of two-line rows. - if line.strip() != "" and not self._is_header_row(line) and not two_line_row_start(line): - row_cnt += 1 + for _ in self._iter_records(f): + row_cnt += 1 return row_cnt def _read_rows_internal(self, block_start=0, block_size=None, **kwargs): @@ -218,36 +309,14 @@ def _read_rows_internal(self, block_start=0, block_size=None, **kwargs): The data read in from the file. """ records = [] + block_end = block_start + block_size if block_size is not None else None with open(self.filename, "r") as f: - curr_block = 0 - block_end = block_start + block_size if block_size is not None else None - prev_line = None - check_header = True - for curr_line in f: - if check_header and self._is_header_row(curr_line): - continue - else: - check_header = False + for curr_block, (main_line, second_line) in enumerate(self._iter_records(f)): if block_end is not None and curr_block >= block_end: # We have read enough rows from the file. break - if two_line_row_start(curr_line): - # We have a two-line row. We will save our current line - # and wait for the next line to merge them as a single row to process. - prev_line = curr_line - continue - - # Process our current MPC Obs80 row. if curr_block >= block_start: - if prev_line is not None: - # We have a two-line row to process. - records.append(self.convert_obs80(prev_line, second_line=curr_line)) - # Remove the previous line so we don't process it again. - prev_line = None - else: - # We have a single line to process. - records.append(self.convert_obs80(curr_line)) - curr_block += 1 + records.append(self.convert_obs80(main_line, second_line=second_line)) return np.array(records, dtype=self.output_dtype) @@ -259,17 +328,8 @@ def _build_id_map(self): obj_ids = [] with open(self.filename, "r") as f: - check_header = True - for curr_line in f: - if check_header and self._is_header_row(curr_line): - continue - else: - check_header = False - if two_line_row_start(curr_line): - # We have a two-line row, so skip it and only - # add the object ID from the final row. - continue - obj_id = self.get_obs80_id(curr_line) + for main_line, _second_line in self._iter_records(f): + obj_id = self.get_obs80_id(main_line) obj_ids.append(obj_id) # Count the number of times we see this object ID. self.obj_id_counts[obj_id] = self.obj_id_counts.get(obj_id, 0) + 1 @@ -299,36 +359,13 @@ def _read_objects_internal(self, obj_ids, **kwargs): skipped_rows = ~np.isin(self.obj_id_table[self._primary_id_column_name], obj_ids) records = [] - # The index of the current row we are processing to check against skipped_rows. - # We start at -1 because we will increment it before processing the first row. - curr_row_idx = -1 - prev_line = None - check_header = True with open(self.filename, "r") as f: - for curr_line in f: - if check_header and self._is_header_row(curr_line): - continue - else: - check_header = False - if two_line_row_start(curr_line): - # We have a two-line row. We will save our current line - # and wait for the next line to merge them as a single row to process. - prev_line = curr_line - continue - - # We're at a potentially processable row, so increment our index. - curr_row_idx += 1 + # _iter_records enumerates records in the same order as _build_id_map, + # so curr_row_idx lines up with skipped_rows. + for curr_row_idx, (main_line, second_line) in enumerate(self._iter_records(f)): if skipped_rows[curr_row_idx]: continue - - # Process our current MPC Obs80 row. - if prev_line is not None: - records.append(self.convert_obs80(prev_line, curr_line)) - # Remove the previous line so we don't process it again. - prev_line = None - else: - # Our row is a single line to process. - records.append(self.convert_obs80(curr_line)) + records.append(self.convert_obs80(main_line, second_line=second_line)) return np.array(records, dtype=self.output_dtype) def _process_and_validate_input_table(self, input_table, **kwargs): diff --git a/tests/data/j4767_wise_excerpt.txt b/tests/data/j4767_wise_excerpt.txt new file mode 100644 index 00000000..d635a99c --- /dev/null +++ b/tests/data/j4767_wise_excerpt.txt @@ -0,0 +1,25 @@ +j4767K10F61M S2010 03 26.79569 06 54 24.99 +42 59 21.9 L~0KAAC51 +j4767K10F61M s2010 03 26.79569 1 + 504.4738 + 5046.9579 + 4682.8243 ~0KAAC51 +j4767K10F61M S2010 03 26.92800 06 54 29.02 +42 58 38.6 L~0FofC51 +j4767K10F61M s2010 03 26.92800 1 + 491.8912 + 5054.7590 + 4675.9257 ~0FofC51 +j4767K10F61M* x2010 03 26.92800 06 54 29.05 +42 58 36.4 L~0FofC51 +j4767K10F61M S2010 03 27.06030 06 54 33.00 +42 57 52.4 L~0FofC51 +j4767K10F61M s2010 03 27.06030 1 + 478.6864 + 5063.2272 + 4667.7874 ~0FofC51 +j4767K10F61M S2010 03 27.32503 06 54 41.10 +42 56 23.0 L~0FofC51 +j4767K10F61M s2010 03 27.32503 1 + 463.6296 + 5022.0464 + 4713.4870 ~0FofC51 +j4767K10F61M S2010 03 27.39112306 54 43.17 +42 56 02.4 L~0FofC51 +j4767K10F61M s2010 03 27.3911231 + 452.2530 + 5054.6265 + 4679.9047 ~0FofC51 +j4767 S2010 03 27.45733806 54 45.173+42 55 39.86 L~2heaC51 +j4767 s2010 03 27.4573381 + 450.7376 + 5030.2958 + 4706.2544 ~2heaC51 +j4767K10F61M S2010 03 27.52342606 54 47.22 +42 55 18.0 L~0FofC51 +j4767K10F61M s2010 03 27.5234261 + 439.3189 + 5063.4199 + 4671.8991 ~0FofC51 +j4767K10F61M S2010 03 27.58964206 54 49.28 +42 54 55.9 L~0KAAC51 +j4767K10F61M s2010 03 27.5896421 + 438.0261 + 5039.2367 + 4697.8972 ~0KAAC51 +j4767K10F61M S2010 03 27.65573006 54 51.37 +42 54 35.2 L~0FofC51 +j4767K10F61M s2010 03 27.6557301 + 426.6286 + 5071.8340 + 4663.6895 ~0FofC51 +j4767K10F61M S2010 03 27.78804 06 54 55.84 +42 53 48.6 L~0FofC51 +j4767K10F61M s2010 03 27.78804 1 + 413.9032 + 5079.2900 + 4656.6791 ~0FofC51 +j4767K10F61M S2010 03 27.92047 06 54 59.64 +42 53 04.3 L~0KAAC51 +j4767K10F61M s2010 03 27.92047 1 + 411.6025 + 5029.9274 + 4710.1435 ~0KAAC51 +j4767K10F61M S2010 03 28.05277 06 55 03.89 +42 52 21.2 L~0KAAC51 +j4767K10F61M s2010 03 28.05277 1 + 398.3093 + 5038.1741 + 4702.1523 ~0KAAC51 diff --git a/tests/layup/test_Obs80Reader.py b/tests/layup/test_Obs80Reader.py index fb6f1c07..9bb9a855 100644 --- a/tests/layup/test_Obs80Reader.py +++ b/tests/layup/test_Obs80Reader.py @@ -4,12 +4,35 @@ from layup.utilities.data_utilities_for_tests import get_test_filepath from layup.utilities.file_io.Obs80Reader import Obs80DataReader +# A well-formed satellite two-line record (S astrometry line + its lower-case s +# observer-position line) and a second distinct one, plus a normal ground-based +# single-line observation. Column layout matches the real C51/WISE records in +# tests/data/03666.txt. +_SAT1_S = "03666 S2015 08 23.89823 03 55 05.36 +17 52 12.2 L~1WXlC51" +_SAT1_s = "03666 s2015 08 23.89823 1 + 1788.6473 + 6184.6473 + 2386.0656 ~1WXlC51" +_SAT2_S = "03666 S2015 08 24.10000 03 55 10.00 +17 53 00.0 L~1WXlC51" +_SAT2_s = "03666 s2015 08 24.10000 1 + 1700.0000 + 6200.0000 + 2400.0000 ~1WXlC51" +_GROUND = " DES0024* C2016 10 02.18440 00 35 08.563+01 31 50.69 23.27i W84" +# A deleted/replaced observation (note 2 code 'x'), a real j4767 WISE/C51 line: +# same exposure as a kept satellite obs but superseded, and carrying no observer +# position of its own. +_DELETED = "j4767K10F61M* x2010 03 26.92800 06 54 29.05 +42 58 36.4 L~0FofC51" + + +def _write(tmp_path, lines): + fp = tmp_path / "obs80.txt" + fp.write_text("\n".join(lines) + "\n") + return str(fp) + def test_row_count(): """Test reading in an MPC Obs80 data filer and reading in the correct number of rows.""" reader = Obs80DataReader(get_test_filepath("03666.txt")) row_count = reader.get_row_count() - assert row_count == 4313 + # 03666.txt contains one deleted observation (a 1938 photographic plate + # flagged with note 2 'X'), which is skipped, so the record count is one + # fewer than the number of astrometry lines. + assert row_count == 4312 reader = Obs80DataReader(get_test_filepath("newy6.txt")) row_count = reader.get_row_count() @@ -121,3 +144,92 @@ def test_read_obs_pos_units(): bad_second_line = second_line[:32] + "3" + second_line[33:] with pytest.raises(ValueError): reader.convert_obs80(first_line, bad_second_line) + + +def test_clean_satellite_pairs_have_positions(tmp_path): + """Baseline: two well-formed satellite records parse to two rows, each + carrying its ICRF_KM observatory position.""" + reader = Obs80DataReader(_write(tmp_path, [_SAT1_S, _SAT1_s, _SAT2_S, _SAT2_s])) + data = reader.read_rows() + assert len(data) == 2 == reader.get_row_count() + assert list(data["sys"]) == ["ICRF_KM", "ICRF_KM"] + assert not np.isnan(data["pos1"]).any() + + +def test_duplicate_continuation_line_is_dropped(tmp_path): + """Regression for the WISE/C51 orphan (Rubin catalog run, object j4767): a + duplicated ``s`` continuation line must not be emitted as a standalone, + positionless observation whose position columns get misread as RA/Dec and + which then falls back to a JPL Horizons lookup. The good pair is kept; the + stray duplicate is skipped.""" + lines = [_SAT1_S, _SAT1_s, _SAT1_s, _SAT2_S, _SAT2_s] # note: _SAT1_s appears twice + reader = Obs80DataReader(_write(tmp_path, lines)) + data = reader.read_rows() + # Only the two real records survive -- no positionless orphan. + assert len(data) == 2 == reader.get_row_count() + assert list(data["sys"]) == ["ICRF_KM", "ICRF_KM"] + assert not np.isnan(data["pos1"]).any() + # The orphan would have duplicated _SAT1_s's timestamp; confirm it is gone. + assert len(set(data["obsTime"])) == 2 + + +def test_leading_orphan_continuation_is_skipped(tmp_path): + """A continuation line with no preceding first line (e.g. a file sliced in + the middle of a two-line record) is skipped rather than mis-read.""" + reader = Obs80DataReader(_write(tmp_path, [_SAT1_s, _SAT2_S, _SAT2_s])) + data = reader.read_rows() + assert len(data) == 1 == reader.get_row_count() + assert data["sys"][0] == "ICRF_KM" + assert not np.isnan(data["pos1"]).any() + + +def test_first_line_missing_continuation_is_dropped(tmp_path): + """An S first line whose s continuation is missing must not be paired with + the next unrelated observation; it is dropped and the following record is + read on its own.""" + reader = Obs80DataReader(_write(tmp_path, [_SAT1_S, _GROUND, _SAT2_S, _SAT2_s])) + data = reader.read_rows() + assert len(data) == 2 == reader.get_row_count() + # The surviving records: the ground-based single obs (no observer position) + # and the intact satellite pair. + assert sorted(data["stn"]) == ["C51", "W84"] + sat = data[data["stn"] == "C51"][0] + assert sat["sys"] == "ICRF_KM" and not np.isnan(sat["pos1"]) + + +def test_deleted_observation_is_skipped(tmp_path): + """A note-2 'x' (deleted/replaced) observation must be skipped entirely, not + emitted as a positionless record. This is the WISE/C51 orphan that surfaced + in the full MPC-catalog fit (object j4767): a deleted satellite astrometry + line has no observer position, so reading it produced a sys='' row that fell + back to a per-row JPL Horizons lookup.""" + reader = Obs80DataReader(_write(tmp_path, [_SAT1_S, _SAT1_s, _DELETED, _SAT2_S, _SAT2_s])) + data = reader.read_rows() + assert len(data) == 2 == reader.get_row_count() + assert list(data["sys"]) == ["ICRF_KM", "ICRF_KM"] + assert not np.isnan(data["pos1"]).any() + + +def test_j4767_wise_excerpt_no_positionless_satellite(): + """Real-data regression: the WISE/C51 block of MPC object j4767 has 12 + satellite S/s pairs and one deleted 'x' line at the same timestamp as a kept + obs. Every emitted C51 record must carry its ICRF_KM observer position; none + may be positionless (which is what triggered the Horizons fallback).""" + reader = Obs80DataReader(get_test_filepath("j4767_wise_excerpt.txt")) + data = reader.read_rows() + assert len(data) == 12 == reader.get_row_count() + assert set(data["stn"]) == {"C51"} + assert list(data["sys"]) == ["ICRF_KM"] * 12 + assert not np.isnan(data["pos1"]).any() + + +def test_desync_count_matches_read_and_objects(tmp_path): + """get_row_count, read_rows and read_objects must all agree on the record + set even when the file contains a desyncing orphan continuation line.""" + lines = [_SAT1_S, _SAT1_s, _SAT1_s, _GROUND, _SAT2_S, _SAT2_s] + reader = Obs80DataReader(_write(tmp_path, lines), primary_id_column_name="provID") + data = reader.read_rows() + assert reader.get_row_count() == len(data) == 3 + # read_objects over every id present returns exactly the same rows. + all_ids = list(set(data["provID"])) + assert len(reader.read_objects(all_ids)) == len(data) From d47db3f1d739f2e62ba6e49bfd87a09aa0aa7f23 Mon Sep 17 00:00:00 2001 From: Matthew Holman Date: Mon, 6 Jul 2026 07:46:30 -0400 Subject: [PATCH 2/3] Guard _iter_records against blank/truncated lines (issue #407) _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) --- src/layup/utilities/file_io/Obs80Reader.py | 4 ++++ tests/layup/test_Obs80Reader.py | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/src/layup/utilities/file_io/Obs80Reader.py b/src/layup/utilities/file_io/Obs80Reader.py index b5908884..dae844b0 100644 --- a/src/layup/utilities/file_io/Obs80Reader.py +++ b/src/layup/utilities/file_io/Obs80Reader.py @@ -245,6 +245,10 @@ def _iter_records(self, f): if check_header and self._is_header_row(line): continue check_header = False + # Skip blank / truncated lines (issue #407): note 2 is at column 15, + # so anything shorter cannot be an obs80 record. + if len(line.rstrip("\n")) < 15: + continue if deleted_observation(line): # A deleted/replaced observation. If it was the first line of a # two-line record its continuation is now orphaned and will be diff --git a/tests/layup/test_Obs80Reader.py b/tests/layup/test_Obs80Reader.py index 9bb9a855..36e6bae9 100644 --- a/tests/layup/test_Obs80Reader.py +++ b/tests/layup/test_Obs80Reader.py @@ -223,6 +223,15 @@ def test_j4767_wise_excerpt_no_positionless_satellite(): assert not np.isnan(data["pos1"]).any() +def test_blank_and_truncated_lines_are_skipped(tmp_path): + """Blank or truncated lines (shorter than the note-2 column) must be skipped, + not fed to convert_obs80 where they raise (issue #407).""" + reader = Obs80DataReader(_write(tmp_path, [_SAT1_S, _SAT1_s, "", "03666 short", _GROUND])) + data = reader.read_rows() + assert len(data) == 2 == reader.get_row_count() + assert sorted(data["stn"]) == ["C51", "W84"] + + def test_desync_count_matches_read_and_objects(tmp_path): """get_row_count, read_rows and read_objects must all agree on the record set even when the file contains a desyncing orphan continuation line.""" From 432bbb30790639783dfce68635db29775fd6261b Mon Sep 17 00:00:00 2001 From: Matthew Holman Date: Mon, 6 Jul 2026 10:30:07 -0400 Subject: [PATCH 3/3] Fold in #411's by-type two-line dispatch (roving/radar); subsumes #411 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) --- src/layup/utilities/file_io/Obs80Reader.py | 47 +++++++++++++++++----- tests/data/obs80_two_line_records.txt | 8 ++++ tests/layup/test_Obs80Reader.py | 42 +++++++++++++++++++ 3 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 tests/data/obs80_two_line_records.txt diff --git a/src/layup/utilities/file_io/Obs80Reader.py b/src/layup/utilities/file_io/Obs80Reader.py index dae844b0..3d80cc2a 100644 --- a/src/layup/utilities/file_io/Obs80Reader.py +++ b/src/layup/utilities/file_io/Obs80Reader.py @@ -204,8 +204,9 @@ def _is_header_row(self, line): True if the line is a header row, False otherwise. """ # We know a line is a header row if it starts with an all-caps 3 letter code - # followed by a space. - return line[0:3].isupper() and line[3] == " " + # followed by a space. The length guard tolerates a blank/short leading + # line (issue #407). + return len(line) >= 4 and line[0:3].isupper() and line[3] == " " def get_reader_info(self): """Return a string identifying the current reader name @@ -472,17 +473,41 @@ def convert_obs80(self, line, second_line=None): raise ValueError( f"Observatory codes do not match in the second line provided for the observatory position. {obs_code} and {second_line[77:80].rstrip()}" ) - unit_flag = second_line[32:34].strip() - if unit_flag in ["1", "2"]: - ades_sys = "ICRF_KM" if unit_flag == "1" else "ICRF_AU" - # For each coordinate, the first character is a sign (+/-) and the next 10 characters are the value. - obs_geo_x = float(second_line[34] + second_line[35:45].strip()) - obs_geo_y = float(second_line[46] + second_line[47:57].strip()) - obs_geo_z = float(second_line[58] + second_line[59:69].strip()) - else: + # The three two-line record types share the S/R/V mechanism but carry + # different second-line payloads, so dispatch on the first line's note2 + # rather than assuming a satellite geocentric position (issue #402). + record_type = line[14] + if record_type == "S": + # Satellite: geocentric equatorial (ICRF) position, km or AU. + unit_flag = second_line[32:34].strip() + if unit_flag in ["1", "2"]: + ades_sys = "ICRF_KM" if unit_flag == "1" else "ICRF_AU" + # For each coordinate, the first character is a sign (+/-) and the next 10 characters are the value. + obs_geo_x = float(second_line[34] + second_line[35:45].strip()) + obs_geo_y = float(second_line[46] + second_line[47:57].strip()) + obs_geo_z = float(second_line[58] + second_line[59:69].strip()) + else: + raise ValueError( + f"Unknown observatory position unit flag '{unit_flag}' in the second line of obs80 data. Should be '1' (km) or '2' (AU)." + ) + elif record_type == "V": + # Roving observer: geodetic East longitude / latitude (degrees) and + # altitude (metres) on the WGS84 ellipsoid. We capture the position + # and its frame here; the geodetic -> geocentric-ICRF conversion is + # the observatory's job (issue #282). + ades_sys = "WGS84" + obs_geo_x = float(second_line[33:45]) # East longitude (deg) + obs_geo_y = float(second_line[45:56]) # latitude (deg) + obs_geo_z = float(second_line[56:67]) # altitude (m) + elif record_type == "R": + # Radar: the second line carries range/Doppler, not an observer + # position. Radar is ingested via ADES delay/doppler, not here. raise ValueError( - f"Unknown observatory position unit flag '{unit_flag}' in the second line of obs80 data. Should be '1' (km) or '2' (AU)." + f"Radar (R/r) obs80 two-line records are not supported by Obs80DataReader " + f"(object {obj_id}); ingest radar via ADES delay/doppler columns." ) + else: + raise ValueError(f"Unexpected two-line record type '{record_type}' for object {obj_id}.") return ( obj_id, diff --git a/tests/data/obs80_two_line_records.txt b/tests/data/obs80_two_line_records.txt new file mode 100644 index 00000000..a28a0370 --- /dev/null +++ b/tests/data/obs80_two_line_records.txt @@ -0,0 +1,8 @@ +00433 A1893 10 29.4132 06 08 59.32 +53 39 04.2 HA053802 + +00433 S2011 10 23.34124006 53 03.495+46 43 06.69 X~7lwF275 +00433 s2011 10 23.3412401 + 4353.0030 - 481.6100 + 1382.3400 ~7lwF275 + +00433 V2023 08 26.19193220 55 41.10 -08 18 29.6 15.1 VV~7811270 +00433 v2023 08 26.1919321 237.76096 +38.11385 0 ~7811270 + diff --git a/tests/layup/test_Obs80Reader.py b/tests/layup/test_Obs80Reader.py index 36e6bae9..ef300b85 100644 --- a/tests/layup/test_Obs80Reader.py +++ b/tests/layup/test_Obs80Reader.py @@ -146,6 +146,48 @@ def test_read_obs_pos_units(): reader.convert_obs80(first_line, bad_second_line) +def test_read_roving_observer_position(): + """A roving-observer (V/v) two-line record carries geodetic longitude/latitude + (deg) and altitude (m) on WGS84 -- not a geocentric satellite position. The + reader must dispatch on the record type and parse it as such rather than + mis-reading it as a satellite position (issue #402).""" + reader = Obs80DataReader(get_test_filepath("03666.txt")) + + first_line = "00433 V2023 08 26.19193220 55 41.10 -08 18 29.6 15.1 VV~7811270" + second_line = "00433 v2023 08 26.1919321 237.76096 +38.11385 0 ~7811270" + + data = reader.convert_obs80(first_line, second_line) + assert data[-5] == "WGS84" + assert data[-4] == 399 + assert data[-3] == pytest.approx(237.76096, rel=1e-6) # East longitude (deg) + assert data[-2] == pytest.approx(38.11385, rel=1e-6) # latitude (deg) + assert data[-1] == pytest.approx(0.0, abs=1e-9) # altitude (m) + + +def test_radar_two_line_record_raises(): + """Radar (R/r) obs80 two-line records are not observer-position lines; the + reader should raise a clear error rather than mis-parsing them (issue #402).""" + reader = Obs80DataReader(get_test_filepath("03666.txt")) + r_first = "00433 R2011 10 23.34124006 53 03.495+46 43 06.69 X~7lwF275" + r_second = "00433 r2011 10 23.3412401 + 4353.0030 - 481.6100 + 1382.3400 ~7lwF275" + with pytest.raises(ValueError, match="[Rr]adar"): + reader.convert_obs80(r_first, r_second) + + +def test_reader_skips_blank_lines_and_parses_two_line_records(): + """Blank/truncated lines must be skipped, not crash the reader (issue #407), + and a mixed file of single-line, satellite, and roving records reads cleanly.""" + reader = Obs80DataReader(get_test_filepath("obs80_two_line_records.txt"), primary_id_column_name="provID") + data = reader.read_rows() + # single optical + satellite + roving = 3 records; the blank lines are skipped. + assert len(data) == 3 + assert list(data["sys"]) == ["", "ICRF_KM", "WGS84"] + # the roving record kept its geodetic position + roving = data[data["sys"] == "WGS84"][0] + assert roving["pos1"] == pytest.approx(237.76096, rel=1e-6) + assert roving["pos3"] == pytest.approx(0.0, abs=1e-9) + + def test_clean_satellite_pairs_have_positions(tmp_path): """Baseline: two well-formed satellite records parse to two rows, each carrying its ICRF_KM observatory position."""