From 0bd00b66f93eb123da645e7e4b0bc4c6b594846c Mon Sep 17 00:00:00 2001 From: Robert French Date: Tue, 11 Aug 2026 12:50:46 -0700 Subject: [PATCH 1/9] critiques: add the 2026-08-10 bundle and repository state review Full analysis of the repository state with emphasis on PDS4 bundle generation, covering the bundle generated on 2026-08-10, the generator and templates, the user guide sources and shipped PDF, the generation logs, and the on-disk pipeline data feeding the bundle. The central finding is that the current bundle was built from pre-fix code against pre-fix data: neither the f-ring code_review_fixes branch nor the rms-csmithing float64-time fix is merged, and the mosaic and background files predate the latter. New blockers found on top of that: - ISS_287RI_PROPRETRG001_PRIME crashed mid-generation on a source PDS3 label whose keyword block is duplicated (pdsparser suffixes duplicate keys), so the bundle ships 6 missing reproj products, a phantom inventory row in two collections, and dangling source-image references in two mosaic tables. - 299 of 305 background-subtracted mosaics archive masked pixels as valid I/F instead of the -999 sentinel, because the generator discards the npz mask. - Mosaic per-column times are float32-quantized, propagating into the derived inertial longitude, core radius, and true anomaly columns. - A __pycache__ directory inside the document collection, a dangling external LIDVID, and several unresolved external reference decisions. - User guide gaps: the array radial-axis direction, the definition of inertial longitude, the background-limit sign convention, and the IMGID naming convention are undocumented, and every numeric excerpt in section 4 disagrees with the regenerated bundle. Also moves the superseded 2026-07-19 and 2026-07-21 critiques into critiques/archive/, and records the 2026-08-10 generator traceback in the pds4_bundle_gen TODO along with the bundle.lblx item marked done. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-07-19.md | 0 .../pds4-bundle-gen-critique-2026-07-21.md | 0 .../pds4-bundle-gen-critique-2026-08-10.md | 503 ++++++++++++++++++ pds4_bundle_gen/TODO.txt | 2 +- 4 files changed, 504 insertions(+), 1 deletion(-) rename critiques/{ => archive}/pds4-bundle-gen-critique-2026-07-19.md (100%) rename critiques/{ => archive}/pds4-bundle-gen-critique-2026-07-21.md (100%) create mode 100644 critiques/pds4-bundle-gen-critique-2026-08-10.md diff --git a/critiques/pds4-bundle-gen-critique-2026-07-19.md b/critiques/archive/pds4-bundle-gen-critique-2026-07-19.md similarity index 100% rename from critiques/pds4-bundle-gen-critique-2026-07-19.md rename to critiques/archive/pds4-bundle-gen-critique-2026-07-19.md diff --git a/critiques/pds4-bundle-gen-critique-2026-07-21.md b/critiques/archive/pds4-bundle-gen-critique-2026-07-21.md similarity index 100% rename from critiques/pds4-bundle-gen-critique-2026-07-21.md rename to critiques/archive/pds4-bundle-gen-critique-2026-07-21.md diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md new file mode 100644 index 0000000..137cc3a --- /dev/null +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -0,0 +1,503 @@ +# PDS4 bundle & repository state critique — 2026-08-10 + +**Scope.** Complete analysis of the repository state with emphasis on PDS4 bundle +generation: the generated bundle at `pds4_bundle_gen/bundle` (→ +`/data/fring-bundles/pds4`, built today 2026-08-10 13:52–16:06), the generator +(`pds4_bundle_gen/generate_pds4_files.py` + `templates/`), the user guide +(`user_guide/sections/*.tex` and the shipped PDF), the generation logs +(`/data/fring-bundles/logs/`), and the on-disk pipeline data feeding the bundle. + +**Method.** Seven parallel review passes plus orchestrator verification: +- Every inventory row of all 11 collections reconciled against disk (42,107 + `.lblx`, 21,045 `.img`, 84,180 `.png`, 21,658 `.tab`, 20,436 `.txt`). +- All 42,107 labels parsed as XML; 77 labels (all collection/index/support labels + plus 60 sampled products) validated against the official 1O00-series XSDs + (schematron not checked — `validate` still required). +- Byte-level verification of labels vs files (file_size, md5, record counts, + field offsets) for all data labels globally and 60 products in depth, + including numeric reads of the binary arrays. +- All PNG dimensions parsed from IHDR and checked against labels and code. +- Live PDS registry and RMS-holdings queries for every external LID/LIDVID. +- Every guide section read line-by-line and every concrete guide claim + (listings, field tables, counts, verbatim excerpts, example scripts) + re-executed or re-derived against the actual bundle. +- Key claims independently re-verified by the orchestrator (float32 ET + quantization, bkgnd-sub mask discard, phantom inventory row). + +**Supersedes** the 2026-07-19 and 2026-07-21 critiques (moved to +`critiques/archive/`) and complements `critiques/code-review-2026-08-04.md`, +which lives on the **unmerged** `code_review_fixes` branch. + +Accepted non-issues (per prior user feedback) are not re-flagged: hardcoded +paths; `metadata['time']` = image midtime; imshow half-pixel extent; signed +core−Prometheus sort; contributor sequence 1,2,2,2; incidence angle treated as +constant per mosaic (min/max = mean). + +--- + +## 1. State of the repository — the context for everything below + +The single most important fact: **today's bundle was built from pre-fix code +against pre-fix data.** + +1. **`code_review_fixes` is not merged.** main is at `9612b2e`; the branch + carries 9 commits (52cb449 … a279009) containing every fix from the + 2026-08-04 review — including the PDS4 label-validation fixes + (`vertical_display_direction`, `document_standard_id`, mission-phase + boundaries, inventory-integrity gating, template/example fixes) and the + guide corrections (1a85209). None of it is in this bundle. +2. **The rms-csmithing float64 time fix is not merged and the data was never + rebuilt.** `/seti/nav/rms-csmithing` has `fix_mosaic_time_float64` + (f156fdb) checked out but unmerged/unpushed, and every mosaic/bkgnd file + under `/data/cb-results/fring/ring_mosaic/` is dated **Jul 23** — before + the fix. The bundle therefore inherits float32-quantized times (§3.2). +3. Working tree: only the user's uncommitted `pds4_bundle_gen/TODO.txt` edit + (which now also contains a pasted copy of today's crash traceback). +4. Generation logs for today's run: 1 uncaught exception (§3.1) and 73 + Prometheus/Pandora visibility warnings (§4.4); nothing else. + +**Consequence:** even with zero new findings, this bundle could not be final — +it must be regenerated after the branch merges and the data is rebuilt. The +findings below are therefore split into (a) issues the existing branch already +fixes, (b) new issues needing new code/text changes, and (c) decisions. + +--- + +## 2. Release blockers — summary + +In recommended fix order (details in the cited sections): + +| # | Blocker | Where fixed | +|---|---------|-------------| +| B1 | `code_review_fixes` unmerged → all known label-validation errors present in this build | merge branch (§6) | +| B2 | float32-quantized `rings:observed_event_tdb` in all mosaic params tables (±8–32 s → up to ~0.2° derived-longitude error) | merge rms-csmithing fix, **rebuild mosaics + bkgnd** (not reproject), regenerate (§3.2) | +| B3 | 299/305 bkgnd-sub mosaics archive masked/bad pixels as valid-looking I/F instead of −999 | new generator (or bkgnd-writer) fix (§3.3) | +| B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | new generator fix (duplicate-keyword tolerance) + regenerate obsid (§3.1) | +| B5 | `__pycache__` with 5 `.pyc` files inside `document/user_guide/` | delete; prune in packaging (§4.1) | +| B6 | Dangling external LIDVID `iss-data-user-guide::1.0` (only `::1.1` exists) | one-character template/CSV fix (§4.2) | +| B7 | Guide: missing core concepts (array axis direction, inertial-longitude definition, bkgnd-limit semantics, IMGID convention) | guide edits (§4.7–4.10; §4.6 withdrawn) | +| B8 | Guide: every §4 verbatim numeric excerpt and the product counts disagree with the real bundle | re-capture from the **final** bundle, rebuild PDF (§4.11) | +| B9 | Moon `Target_Identification` wrong for the 12+8 "visually confirmed but geometrically rejected" mosaics; "not visually confirmed" disclaimer emitted even when confirmed | new generator fix / policy decision (§4.4) | +| B10 | Decisions required: `data_calibrated` forward references (§4.3), source-product VIDs `::1.0` vs PDS3 versions 2–9 (§4.5), xml_schema LIDVIDs (§4.12), open TODO items (§7) | user | + +--- + +## 3. Critical findings + +### 3.1 ISS_287RI_PROPRETRG001_PRIME: generation crashed mid-obsid; the bundle ships an incomplete, self-inconsistent product set — NEW root cause + +Today's run aborted this obsid's reproj stage with an uncaught +`KeyError: 'SPACECRAFT_CLOCK_START_COUNT'` at `generate_pds4_files.py:1748`. + +**Root cause (new bug, distinct from the known §2.6 gating issue).** The source +PDS3 label +`/data/pdsdata/holdings/calibrated/COISS_2xxx/COISS_2115/data/1880794265_1880940914/N1880796883_3_CALIB.LBL` +— the only version-`_3` image in the obsid — is malformed at the archive level: +a ~14-keyword block (`SEQUENCE_TITLE` … `TELEMETRY_FORMAT_ID`, including all +`SPACECRAFT_CLOCK_*`, `START_TIME`, `STOP_TIME`, `TARGET_*`) appears **twice** +at top level with identical values. `pdsparser` renames duplicated keys to +`KEY_1`/`KEY_2`, so no unsuffixed `SPACECRAFT_CLOCK_START_COUNT` exists. +`xml_add_pds3_label_info` (lines 1740–1747) catches only `FileNotFoundError` +and `pyparsing.ParseException`, so the `KeyError` escapes to the top-level bare +`except`, which logs and skips the rest of the obsid. + +- **Scope:** a scripted scan of all 21,046 unique source labels across all 305 + mosaics found exactly this one duplicated label. No other obsid affected. +- **Regression:** the previous build + (`/data/fring-bundles/cassini_iss_fring_mosaics_rsfrench2025.old2`) contains + this product, so a pdsparser/environment change altered duplicate-key + behavior between builds. + +**Shipped damage (all verified in the bundle):** +- 6 of 19 reproj products missing (`1880796883n`, `1880796976n`, `1880797069n`, + `1880797162n`, `1880797255n`, `1880797348n`) from both `data_reproj_img/` and + `browse_reproj_img/`. +- One **phantom Primary inventory row** for `1880796883n…::1.0` in *both* + `collection_data_reproj_img.csv` (row 20,436) and + `collection_browse_reproj_img.csv` — inventory rows are written before + `generate_reproj` runs. Guaranteed `validate` referential-integrity failure. + The other five images have no rows anywhere (silent data loss). +- Both 287RI mosaic products' `*_metadata_src_imgs.tab` list all 19 LIDVIDs — + indexes 13–18 dangle — and the mosaics *use* those images' data (image_index + 13 supplies 32 longitude columns of the params table), so the archived + mosaics permanently reference source products that were never archived. +- `global_mosaic_index.tab`/`global_mosaic_bkg_sub_index.tab` carry + `num_images=19`, `max_image_name=1880797348n` for this obsid while only 13 + reproj products exist. + +**The branch fix is NOT sufficient.** `fe4f8f5`'s per-image guard catches only +`ObsIdFailedException`; this `KeyError` would still escape to the outer generic +`except → continue`, which on the branch would then also skip the obsid's +*mosaic* inventory rows even though the mosaic products were written — the +inverse validate failure (orphan products). Required fix: + +1. Duplicate-tolerant PDS3 lookup: if `key` missing, fall back to `f'{key}_1'` + (log a warning; optionally assert `_1 == _2`). Fixing only the SCLK line is + insufficient — `START_TIME`, `STOP_TIME`, `SEQUENCE_TITLE`, `SHUTTER_*`, + `TARGET_DESC`, `TELEMETRY_FORMAT_ID`, `SOFTWARE_VERSION_ID` are read a few + lines later and are all suffixed in this label. +2. Backstop: convert unexpected exceptions in `xml_add_pds3_label_info` (at + minimum `KeyError`) into `ObsIdFailedException`. +3. On the branch: make the per-image guard survive unexpected exceptions + without aborting the obsid's inventory bookkeeping. +4. Regenerate the obsid (products, both collection CSVs, all three global + indexes). + +### 3.2 float32-quantized times contaminate every mosaic params table — data rebuild required (verified numerically) + +In the shipped bundle, every `rings:observed_event_tdb` value in the sampled +mosaic params tables is *exactly* float32-representable: at ET ≈ 2.1×10⁸ the +representable spacing is 16 s, and consecutive unique ETs in +`iss_029rf_fmovie001_vims_mosaic_metadata_params.tab` differ by 528/544 s +instead of the true smooth cadence. At 2017-era ETs (≈5.5×10⁸ s) the spacing is +64 s. Consequences: per-longitude times are wrong by up to ±8–32 s, and the +generator computes `rings:inertial_ring_longitude`, `core_radius`, +`longitude_ascending_node`/pericenter, and `true_anomaly` **from these +quantized ETs**, so those columns inherit errors up to ~0.2° / ~1 km. +Reprojected-image tables are unaffected (per-image scalar float64 midtime; +verified <0.01° round-trip). + +The fix (`rms-csmithing` `fix_mosaic_time_float64`, f156fdb) exists but the +mosaic (`ring_ui_mosaic.py`) and background (`ring_ui_bkgnd.py`) stages have +not been rerun since (all data files Jul 23). **Reprojection does not need to +be rerun.** Required order: merge the fix → rebuild mosaics → rebuild +backgrounds → regenerate the bundle. + +### 3.3 Background-subtracted mosaics archive masked "bad" pixels as valid I/F — NEW (verified; resolves TODO line 64 in the negative) + +`generate_pds4_files.py:800–806` discards the npz mask +(`metadata['img'].mask = False`) on the assumption that all missing data was +already converted to −999. That assumption is false: in **299 of 305** +production `*-BKGND-SUB-MOSAIC.npz` files there are masked pixels whose stored +value is *not* −999 — e.g. `ISS_007RI_AZSCNLOPH001`: 6,019 of 11,633 masked +pixels carry values −0.0006…0.144 (plausible I/F; independently re-verified). +Mechanism (`mosaics/ring_ui_bkgnd.py:148–153`): `corrected = mosaic_img − +bkgnd_model.data` uses the raw `.data` of the masked background model +(garbage/unfit values at masked pixels); −999 is applied only where the +original pixel was −999 or the entire column is masked. Partially-masked +columns keep unsubtracted/garbage values, which the bundle then archives as +valid data — undetectable by users via `Special_Constants`. + +Fix options: honor the mask in `read_mosaic` (masked → SENTINEL) at bundle +time (simplest, no data rebuild needed beyond §3.2's), and/or sentinel them in +the bkgnd writer. The photometry pipeline reads the mask via `f_ring_util` and +is unaffected; only the archive is wrong. + +--- + +## 4. Major findings + +### 4.1 `__pycache__` inside the document collection — NEW +`document/user_guide/__pycache__/` contains five `.cpython-312.pyc` files +(mtime 20:19, i.e. created *after* generation by running the example scripts in +place). Unlabeled, non-archival → `validate` errors. Delete before delivery and +make the generator/packaging prune `__pycache__` (and never execute the shipped +scripts inside the bundle tree). + +### 4.2 Dangling external LIDVID: `iss-data-user-guide::1.0` — NEW +`document/collection_document.csv:2` and +`miscellaneous/collection_miscellaneous.csv:4` (sources: +`templates/collection_document.csv`, `templates/collection_miscellaneous.csv`) +reference `urn:nasa:pds:cassini_iss_saturn:document:iss-data-user-guide::1.0`. +The PDS registry and the live RMS inventory have only **`::1.1`**; `::1.0` +does not exist → referential-integrity error at ingestion. Change to `::1.1` +(or drop the VID). `bundle.lblx` is safe (LID-only reference). + +### 4.3 All 20,435 reproj labels reference `cassini_iss_saturn:data_calibrated` — DECISION NEEDED +Every reproj label's `Source_Product_Internal` points at +`urn:nasa:pds:cassini_iss_saturn:data_calibrated:_calib::1.0`, but the +archived `cassini_iss_saturn::1.1` bundle contains no `data_calibrated` +collection (only browse_raw/context/data_raw/document/xml_schema; registry +lookups return not-found). If a calibrated-ISS delivery is coordinated with +RMS this is a deliberate forward reference; otherwise every reproj label ships +a dangling source-product LIDVID. Confirm with RMS before final. + +### 4.4 Moon `Target_Identification` policy: geometric test overrides the human "visually confirmed" flags — NEW +`generate_pds4_files.py:1929–1941` warns on disagreement but the geometric +test always wins. Today's 73 warnings decompose as Prometheus 46 geo-True/vis- +False + 12 geo-False/vis-True; Pandora 7 + 8. +- The **geo-False/vis-True** cases yield wrong labels: a moon the author + visually confirmed is omitted from `Target_Identification`. Verified for 4 + obsids: the moon-to-modeled-core separation is 1010.5–1027.6 km — just past + the hard ±1000 km window (`_image_has_satellite`, :934–938), which ignores + the moon's own radius (~40–70 km) and the ~±50 km scatter of the real core + about the Albers model. The visual flag is right; the window is too strict. +- The "presence has not been visually confirmed" sentence (:2014–2025, + :2298–2309) is emitted **unconditionally** — including for the common case + where `observation_list.csv` says `Y`. The archived text then contradicts + the author's own records. +- The same observation can get different target lists in its two mosaic + variants (verified: `iss_105ri_tmapn45lp001_cirs_4` mosaic lists Pandora, + its bkg_sub twin doesn't) because background subtraction drops columns and + changes the antimask. +- Latent: the edge check (:922) uses compressed-array indices with no + wraparound — a moon near corot 0° in a full-360° mosaic is wrongly rejected. + +Suggested: widen the window by moon radius + model tolerance (or let the +visual flag override for mosaics), and condition the disclaimer sentence on +the flag. (Commit 83b46eb's inertial-longitude core radius was verified +correct; residual error ~0.05 km.) + +### 4.5 `Source_Product_Internal` hardcodes `::1.0` while 165 source images are PDS3 versions 2–9 — VERIFY EXTERNALLY +`image_name_to_calib_lidvid` (:1026–1033) always emits `::1.0`. The mosaics use +165 source images with PDS3 version suffix ≥ 2 (v2:37 … v9:2). If the migrated +calibrated bundle assigns VIDs per PDS3 version (RMS convention), those 165 +reproj labels reference superseded versions. Check against the actual +`cassini_iss_saturn` calibrated delivery (not resolvable from this machine); +combine with the §4.3 decision. + +### 4.6 NOT A BUG (withdrawn) — guide's "emission < 90° = lit side" statement +The original finding assumed a north-based emission convention. Per user +confirmation, the standard rings convention is that incidence and emission are +referenced to the lit-side ring-plane normal, so emission < 90° means lit-side +viewing by definition. Verified in the bundle: `rings:mean_incidence_angle` is +63.3°–89.8° (never > 90°) across all 305 mosaics spanning 2004–2017, and mean +emission straddles 90° in both the pre- and post-equinox eras — exactly as the +lit-side convention predicts. The guide statements at 07:93 and 07:187 are +correct as written. (Optional polish only: one sentence in §3 or §7 stating +the lit-side-normal convention for readers from outside the rings community.) + +### 4.7 Guide: the radial (Line) axis direction of the binary arrays is stated nowhere in the archive — NEW +The guide never says row 0 = Δr = −1000 km (inner edge) with radius increasing +along the Line axis (04:163 gives only the range; 03:299 gives the core row). +Meanwhile the shipped labels' `vertical_display_direction` is wrong (known, +unmerged fix), so **no document in the current archive states the true array +orientation**. One sentence at 04:163 fixes the guide half. + +### 4.8 Guide: "inertial longitude" is never defined — NEW +The corot↔inertial formulas (03:279–289) are given, but the zero point and +direction of inertial longitude (J2000 reference, measured how, from where) +appear nowhere; 07:35/37 ("relative to J2000") is the only hint. Without it a +user cannot independently place a moon, verify `true_anomaly`, or compare with +other published F-ring longitudes. Define it once in §3. + +### 4.9 Guide: `bkgnd_lower_limit`/`bkgnd_upper_limit` semantics contradictory — NEW +03:357 calls them "pixel limits"; 07:269–271 calls them "lower ring delta +radius"; the archived value (generator :2163–2168) is actually a positive, +unsigned distance in km from the core (750 ⇒ region Δr −1000…−750 used +interior). Reading "+750" as a signed delta radius puts the background region +on the wrong side of the ring. State: km, unsigned offset interior (lower) / +exterior (upper), background region between that offset and 1000 km. The same +text should replace the identical ambiguity in `templates/global_index.lblx` +(:897, :908). + +### 4.10 Guide: the `IMGID` naming convention is never defined — NEW +Used ~20 times with examples (`1622049830n`, `1874525875w`) but never +explained: digits = Cassini spacecraft clock count, suffix = camera +(`n` NAC / `w` WAC). It is the primary key of every reproj product and the +link back to COISS; a peer reviewer will ask. + +### 4.11 Guide: every §4 verbatim numeric excerpt — and the product counts — disagree with the real bundle — NEW +All sampled values in the guide's excerpts come from a pre-fix build and the +*bundle* is the correct side (agent recomputed the orbit model: +e.g. reproj sample corot 320.08 → inertial 61.809 in both model and bundle; +guide prints 101.729. Label excerpt for `1622049830n`: samples 655 vs actual +649, file_size 1050620 vs 1040996, corot range 286.14/308.60 vs 186.60/199.56, +plus resolution-format drift 3 → 5 decimals). Counts: guide says "20,303 +reprojected images … 302 mosaics"; the bundle has 20,435 archived (20,441 +intended once §3.1 is fixed) and 305 mosaics. **All excerpts and counts must be +re-captured from the final regenerated bundle**, then the PDF rebuilt — do this +last (note the ordering dependency: bundle → excerpts → PDF → document +collection re-copy). + +### 4.12 xml_schema collection pins five LIDVIDs that don't resolve in the PDS registry — NEW +`collection_xml_schema.csv` pins `pds-xml_schema::1.24`, `disp::1.15`, +`geom::1.19`, `rings::1.14`, `cassini::1.18`; none of those exact VIDs exist in +the registry (and newer EN registrations use different LID patterns). The VIDs +look guessed. Safer: LID-only secondary entries (as `collection_context.csv` +already does) or confirm exact LIDVIDs with EN/RMS. + +--- + +## 5. Minor findings + +### Bundle / support collections +1. SPICE collection `Time_Coordinates` stop is `2017-09-01`, before the + bundle's own data stop (2017-09-07) and the kernels' actual coverage + (≈Sep 19). Placeholder dates on the same line as the known leftover MJTM + comment (`collection_spice_kernels.lblx:136–139`). +2. `mission.cassini-huygens::1.4` cited; registry latest is `::1.5` (valid but + stale at delivery time) — `collection_document.csv:6`, + `collection_miscellaneous.csv:8`. +3. Inventory LIDVID-style inconsistency: `collection_context.csv` uses bare + LIDs for secondary members; document/miscellaneous CSVs use `::1.x` for the + same products. Both legal; normalize. +4. Cosmetic: trailing-period inconsistency in collection titles; + "Initial version" with/without period across `Modification_Detail`s; stray + tab at `bundle.lblx:131`. + +### Generator / templates (main) +5. 559/20,435 reproj "full" browse PNGs are narrower than their data product + (interior gap columns dropped, disclosed in the description text) — but the + same description's size list says "equal in size to the reprojected image", + strictly false for these 2.7%. (Quantifies a known wording issue.) +6. `Special_Constants` comment in `data_mosaic.lblx:233–237` / + `data_reproj_img.lblx:329–333` describes only reproj semantics ("off the + edge of the FOV / transmission error"); for mosaics the dominant meaning is + "no image covered this longitude", and for bkg_sub also "background model + invalid" (esp. once §3.3 is fixed). +7. `spacecraft_clock_count_partition` hardcoded `1` in both data templates + while the generator parses the real `SPACECRAFT_CLOCK_CNT_PARTITION` (dead + key). Always 1 for Cassini; still, use the variable. +8. Global bkg-sub index writes `{upper_limit:4d}` (:2708): a value of −1000 + would be 5 chars and shift the fixed-width record. Current data ranges + 150–905 — latent only. +9. `read_mosaic`/`read_reproj` error paths reference `obsid` (:787, :793, + :870, :876), not a parameter — works only via the top-level loop's global; + `NameError` from any other caller. +10. Inconsistent-camera check (:1720–1725) logs an error but doesn't raise; a + mixed-camera mosaic would silently archive under image 0's camera. +11. Docstring at :1088 shows a garbled example LID + (`…rsfrench2025_mosaic_rsfrench2025…`); emitted LIDs are correct. +12. `pds4_bundle_gen/data_dictionaries/` holds GEOM `19A0` / RINGS `1D00` XSDs + while labels reference GEOM `19B0` / RINGS `1E00` — offline validation + against the local copies tests the wrong LDD versions. +13. Comment drift: `generate_pds4_files.py:88–90` claims reproj browse has only + full+thumb (four sizes are made); `generate_browse` docstring (:2750) files + reproj output under `browse_mosaic/`. +14. Dead-key drift persists (deferred decision last round): `SOFTWARE_VERSION_ID`, + `INST_CMPRS_PARAM`, `MISSION_PHASE_NAME`, `PRODUCT_*` parsed but never + templated. Note: TODO's two "FROM WHERE?" questions have answers — + `ground_software_version_id` = PDS3 `SOFTWARE_VERSION_ID` (already parsed, + currently dead), and `valid_maximum_full_well`/`valid_maximum_DN_sat` = the + two elements of PDS3 `VALID_MAXIMUM`. + +### User guide (beyond §4.6–4.11; file:line into `user_guide/sections/`) +15. 03:178 "Like the M3 mosaics, these mosaics are not particularly useful" — + the not-useful statement was made about the **R** class (03:166), not M3. +16. 03:20 rev described as numeric 000–293, but the bundle contains lettered + rev `ISS_00ARI_SPKMOVPER001_PRIME`; a `[REV][TI]` parser following the + guide mis-splits it. +17. UNIQUENAME acronym list (03:22–42) lacks `SPOKEMOV` (42 mosaics — the + second-most-common stem; the list's `SPK*` entry doesn't match it). +18. R and N classes get no obsid lists while M1–M4 and O do (03:163–172). +19. `nav_quality`/`bkgnd_quality` cross-refs (07:139, 07:257/259) point to §3 + sections that never define the G/F/P grading criteria. +20. Incidence min/max text contradicts itself: 07:91 "not supplied" vs label + excerpt 04:101–103 showing them; 07:185 gives a different rationale than + 07:91. +21. 03:301 promises §5 wraparound-handling examples; §5 has none. Related: + §5's "off-the-shelf" coverage has recipes for labels/.tab/.csv but no + numpy recipe for the binary `.img` (byte order, reshape) — the one format + users can't open with a text editor. +22. 03:263–277 element epoch unstated (ϖ₀, Ω₀ are J2000/ET=0 values; the + corotation epoch 2007-01-01 is defined separately — say so). +23. Negative I/F in bkg-sub mosaics never mentioned (~half of all background + pixels are necessarily < 0 after subtraction). +24. No "cite this bundle as…", no modification-history/errata policy + (frontmatter has V1.0 + DOI only). +25. 02:56 "miscellaneous bundle" → collection. +26. 05:139 links `github.com/SETI/fring-mosaics-bundle-software` — confirm the + repo exists/is populated before archiving the PDF (permanent link). +27. Moon ephemeris source (SPICE kernels in the bundle's own + `spice_kernels` collection) never stated (03:319–322, 07:41–47). +28. §7 gives units for resolutions but not for times ("seconds past J2000 + TDB"), longitudes, or radii — uneven. +29. The suppl.txt verbatim listing (04:180–199) omits the file's actual 3-line + preamble ("This file contains a C-matrix …" + blank line). +30. The known "row i ↔ column i" caveat fix (04:176) should also cover the + mosaic section (04:261–273) and state the mapping + `col = round(longitude / 0.02°)` explicitly. +31. Typos: 03:55 "due the"; 03:87 "each complete or partial final orbit" + (garbled); 04:6 "reprojected image" → images; 04:40 "top-Level"; 05:62 + missing period; 06:65,67 "AASDivision"; 01:18 stray space in + `\textit{User Guide }`; 04:65 stray `\textbf{ }`. + +--- + +## 6. Status of the 2026-08-04 review — all fixes still unmerged + +Everything below is **already fixed on `code_review_fixes`** and confirmed +still present in this build (not re-reported above): mission_phase_name wrong +across boundary-year second halves (verified live: all 2008-07…12 mosaics say +`TOUR`); `vertical_display_direction` "Top to Bottom" in all data labels; +`document_standard_id` "Python" ×5; `collection_document.lblx` Inventory +records 5 vs 10; inventory-row gating (§2.6 — whose reproj half manifested as +§3.1's phantom row); the guide's core-radius equation missing the longitude +term; stale 302/20,303 counts; broken quick-start commands; `<--` comments; +excerpt field order; `local_identifier` mismatch; `core_radius "(constant)"`; +dead customXml href; B3001/B4001 mixed example pairing; "Min and Max True +Anomaly"; src_imgs "name" → LIDVID; med-browse 400-px note; stretch +description. **Merging the branch is the first step of any path to final** — +but note §3.1: the branch's per-image guard needs strengthening, and §4.11: +the branch's re-captured guide numbers must themselves be re-checked against +the final bundle (intended reproj count 20,441, mosaics 305). + +Also unmerged and load-bearing: rms-csmithing `fix_mosaic_time_float64` +(§3.2), and the f-ring branch's quadratic-bkgnd-fit fix (52cb449 — confirmed +harmless to current data: all 305 production backgrounds are degree 1). + +--- + +## 7. TODO.txt assessment (archive-readiness of open items) + +| Item | Verdict | +|---|---| +| Mia: kernels/spice labels, document collection CSV, List_Author/Contributor | Appears done in current templates/bundle. | +| Check navigation of `iss_199rf_fmovie002_prime` | Required before final (data QA). | +| Stars as targets for occultation ('O'/'R') products | Polish, not validity-blocking — the mosaic label comment already discloses the star. Open completeness decision at `:993` XXX (unused images of 'R' obsids excluded from archive). | +| Limit cameras/targets to those present | Largely satisfied (labels are per-camera; collections genuinely contain both moons/cameras). Optional. | +| Cassini field diffs vs original bundle | Content decisions; two "FROM WHERE?" answers found — see §5 item 14. | +| `rings:description` update | Blocked on external dictionary cleanup; defer. | +| Verify sentinels in bkgnd mosaics | **Done — FAILS (§3.3). Must fix.** | +| Wrap-around limits / example labels / bundle.lblx / moons | Marked DONE; consistent with code. | + +--- + +## 8. Verified sound (what does NOT need attention) + +- **Structure:** all 11 collections' inventories reconcile with disk exactly + (sole exception §3.1); zero orphan labels; obsid sets byte-identical across + all six data/browse collections; bundle.lblx member entries complete/valid; + bundle time range exactly spans the data; file census ties out + (21,045/84,180/21,658/20,436/42,107). +- **Labels vs files:** file_size and md5 match for every checked file; all + labels well-formed XML; XSD-valid (sampled, all namespaces, zero version + drift across 42k references); `IEEE754LSBSingle` confirmed correct by + numeric read; date formats uniform `…Z`; no sentinel/N/A leakage. +- **Data self-consistency:** params tables ↔ image columns exact (including + dropped-column bkg_sub cases); label min/max/mean stats match table-derived + values; core radius matches the ellipse model < 1 km; `true_anomaly` = + (inertial − ϖ) % 360; angles/longitudes in range; SCLK/ET orderings correct; + C-matrices orthonormal; suppl header byte math exact. +- **Browse:** all 84,180 PNGs match labels (names, sizes, dims, 8-bit + greyscale); dimension rules match the code everywhere; content sanity-checked. +- **Support:** document label covers all 6 files (MD5s match); PDF intact; + scripts byte-compile and **run to completion against this bundle** (all four + examples, incl. the full 20,435-row index scan); context collection exactly + bijective with label references, all 8 registry-verified; SPICE kernel + complement complete and spanning; global-index field definitions verified + byte-exact against all rows. +- **Constants/formulas:** orbit constants match `f_ring_util` everywhere; + guide arithmetic checks; corotation rate exact; commit 83b46eb (inertial + longitude for satellite core radius) verified correct. +- **Guide:** all filename patterns/trees correct (except `__pycache__`); + params/global-index column tables match actual headers name-for-name; + M1–M4/O lists match index notes exactly; browse sizes match (2f4808c); + LaTeX build clean (no unresolved refs, no overfull boxes). + +--- + +## 9. Recommended path to a final bundle + +1. **Merge** `code_review_fixes` into main; merge rms-csmithing + `fix_mosaic_time_float64` into its main. +2. **New code fixes** (this critique): duplicate-keyword-tolerant PDS3 lookup + + `KeyError` backstop + stronger per-image guard (§3.1); bkgnd-sub mask → + sentinel (§3.3); moon window/disclaimer (§4.4); `iss-data-user-guide::1.1` + (§4.2); xml_schema LIDVID style (§4.12); SPICE Time_Coordinates (§5.1); + `__pycache__` prune (§4.1); small template/text items (§5). +3. **Decisions:** `data_calibrated` forward reference (§4.3); source-product + VIDs for v2–v9 images (§4.5); stars-as-targets and 'R'-obsid completeness; + cassini: dead-key fields (§5.14); `iss_199rf_fmovie002_prime` navigation + check. +4. **Rebuild data:** `ring_ui_mosaic.py` then `ring_ui_bkgnd.py` for all 305 + obsids (reprojection does NOT need rerunning), so mosaic times become + float64. +5. **Guide edits:** §4.7–4.10 concept fixes + §5 minors (on top of the + branch's 1a85209). +6. **Regenerate the bundle**; verify ERRORS.log empty; then **re-capture the + guide's §4 excerpts and counts from this final bundle**, rebuild the PDF, + and re-run (or re-copy) the document collection. +7. Delete any stray files, run PDS4 `validate` (schematron — XSD alone was + checked here), and spot-check the 287RI obsid end-to-end. diff --git a/pds4_bundle_gen/TODO.txt b/pds4_bundle_gen/TODO.txt index 69b485f..3eb8c05 100644 --- a/pds4_bundle_gen/TODO.txt +++ b/pds4_bundle_gen/TODO.txt @@ -5,7 +5,7 @@ Mia's current tasks (8/7/25): - Check navigation of iss_199rf_fmovie002_prime -- Add bundle.xml +DONE - Add bundle.lblx DONE - Deal with Prometheus and Pandora in reprojected images. From 8c1d48ee32b953aa7b93d6d93ff101e67a944bb4 Mon Sep 17 00:00:00 2001 From: Robert French Date: Tue, 11 Aug 2026 16:02:30 -0700 Subject: [PATCH 2/9] critiques: record the code_review_fixes merge in the 2026-08-10 review The review was written while code_review_fixes was still open. It is now merged to main (9fc7045, including the review-feedback revisions in c1168cb), so update the status statements in the summary, the blocker table, section 6, and the recommended path. The findings themselves are unchanged: the bundle on disk was generated from 9612b2e, before the merge, so every fixed item is still present in the archived products until the bundle is regenerated. The rms-csmithing float64 time fix remains unmerged (PR #5) and the mosaic and background data still predate it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-08-10.md | 71 ++++++++++++------- 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md index 137cc3a..d97736a 100644 --- a/critiques/pds4-bundle-gen-critique-2026-08-10.md +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -26,7 +26,8 @@ generation: the generated bundle at `pds4_bundle_gen/bundle` (→ **Supersedes** the 2026-07-19 and 2026-07-21 critiques (moved to `critiques/archive/`) and complements `critiques/code-review-2026-08-04.md`, -which lives on the **unmerged** `code_review_fixes` branch. +which lived on the `code_review_fixes` branch (merged to main 2026-08-11 as +`9fc7045`, *after* this bundle was generated). Accepted non-issues (per prior user feedback) are not re-flagged: hardcoded paths; `metadata['time']` = image midtime; imshow half-pixel extent; signed @@ -40,17 +41,22 @@ constant per mosaic (min/max = mean). The single most important fact: **today's bundle was built from pre-fix code against pre-fix data.** -1. **`code_review_fixes` is not merged.** main is at `9612b2e`; the branch - carries 9 commits (52cb449 … a279009) containing every fix from the - 2026-08-04 review — including the PDS4 label-validation fixes +1. **`code_review_fixes` was not merged when this bundle was built.** + *(Update 2026-08-11: merged to main as `9fc7045`, together with the + review-feedback revisions in `c1168cb`.)* The branch carried every fix from + the 2026-08-04 review — the PDS4 label-validation fixes (`vertical_display_direction`, `document_standard_id`, mission-phase boundaries, inventory-integrity gating, template/example fixes) and the - guide corrections (1a85209). None of it is in this bundle. -2. **The rms-csmithing float64 time fix is not merged and the data was never - rebuilt.** `/seti/nav/rms-csmithing` has `fix_mosaic_time_float64` - (f156fdb) checked out but unmerged/unpushed, and every mosaic/bkgnd file - under `/data/cb-results/fring/ring_mosaic/` is dated **Jul 23** — before - the fix. The bundle therefore inherits float32-quantized times (§3.2). + guide corrections (1a85209). **None of it is in this bundle**, which was + generated from `9612b2e`; a regeneration is required for any of it to + appear in the archive. +2. **The rms-csmithing float64 time fix is still not merged and the data was + never rebuilt.** `/seti/nav/rms-csmithing` has `fix_mosaic_time_float64` + (f156fdb) pushed and open as PR #5, but `origin/main` is still at + `460488f`, and every mosaic/bkgnd file under + `/data/cb-results/fring/ring_mosaic/` is dated **Jul 23** — before the fix. + The bundle therefore inherits float32-quantized times (§3.2). + *(Status re-verified 2026-08-11.)* 3. Working tree: only the user's uncommitted `pds4_bundle_gen/TODO.txt` edit (which now also contains a pasted copy of today's crash traceback). 4. Generation logs for today's run: 1 uncaught exception (§3.1) and 73 @@ -69,7 +75,7 @@ In recommended fix order (details in the cited sections): | # | Blocker | Where fixed | |---|---------|-------------| -| B1 | `code_review_fixes` unmerged → all known label-validation errors present in this build | merge branch (§6) | +| B1 | ~~`code_review_fixes` unmerged~~ → **merged 2026-08-11 (`9fc7045`)**; all known label-validation errors are still present in *this build*, which predates the merge, so a regeneration is required | done — regenerate (§6) | | B2 | float32-quantized `rings:observed_event_tdb` in all mosaic params tables (±8–32 s → up to ~0.2° derived-longitude error) | merge rms-csmithing fix, **rebuild mosaics + bkgnd** (not reproject), regenerate (§3.2) | | B3 | 299/305 bkgnd-sub mosaics archive masked/bad pixels as valid-looking I/F instead of −999 | new generator (or bkgnd-writer) fix (§3.3) | | B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | new generator fix (duplicate-keyword tolerance) + regenerate obsid (§3.1) | @@ -405,10 +411,11 @@ already does) or confirm exact LIDVIDs with EN/RMS. --- -## 6. Status of the 2026-08-04 review — all fixes still unmerged +## 6. Status of the 2026-08-04 review — merged 2026-08-11, bundle predates it -Everything below is **already fixed on `code_review_fixes`** and confirmed -still present in this build (not re-reported above): mission_phase_name wrong +Everything below is **fixed in the code** (`code_review_fixes`, merged to main +2026-08-11 as `9fc7045`) but confirmed **still present in this build**, which +was generated before the merge (not re-reported above): mission_phase_name wrong across boundary-year second halves (verified live: all 2008-07…12 mosaics say `TOUR`); `vertical_display_direction` "Top to Bottom" in all data labels; `document_standard_id` "Python" ×5; `collection_document.lblx` Inventory @@ -418,14 +425,22 @@ term; stale 302/20,303 counts; broken quick-start commands; `<--` comments; excerpt field order; `local_identifier` mismatch; `core_radius "(constant)"`; dead customXml href; B3001/B4001 mixed example pairing; "Min and Max True Anomaly"; src_imgs "name" → LIDVID; med-browse 400-px note; stretch -description. **Merging the branch is the first step of any path to final** — -but note §3.1: the branch's per-image guard needs strengthening, and §4.11: -the branch's re-captured guide numbers must themselves be re-checked against -the final bundle (intended reproj count 20,441, mosaics 305). - -Also unmerged and load-bearing: rms-csmithing `fix_mosaic_time_float64` -(§3.2), and the f-ring branch's quadratic-bkgnd-fit fix (52cb449 — confirmed -harmless to current data: all 305 production backgrounds are degree 1). +description. **None of this reaches the archive until the bundle is +regenerated** — and note §3.1: the merged per-image guard still needs +strengthening, and §4.11: the merged guide numbers must themselves be +re-checked against the final bundle (intended reproj count 20,441, +mosaics 305). + +Still unmerged and load-bearing: rms-csmithing `fix_mosaic_time_float64` +(§3.2, open as PR #5). Also merged with the branch: the quadratic-bkgnd-fit +fix (52cb449 — confirmed harmless to current data: all 305 production +backgrounds are degree 1), and the review-feedback revisions in `c1168cb` +(moons `core_to_*_corot` rename, `--allow-exception` default restored to +True, dead zero-to-sentinel conversion removed after verifying no +reprojected file on disk uses 0, background limits keyed to the radial +resolution, browse sizes re-hardcoded behind geometry asserts, example +scripts simplified, SPICE `Time_Coordinates` removed — which also resolves +§5.1). --- @@ -480,13 +495,15 @@ harmless to current data: all 305 production backgrounds are degree 1). ## 9. Recommended path to a final bundle -1. **Merge** `code_review_fixes` into main; merge rms-csmithing - `fix_mosaic_time_float64` into its main. +1. ~~**Merge** `code_review_fixes` into main~~ — **done 2026-08-11** + (`9fc7045`). Still to do: merge rms-csmithing `fix_mosaic_time_float64` + (PR #5) into its main. 2. **New code fixes** (this critique): duplicate-keyword-tolerant PDS3 lookup + `KeyError` backstop + stronger per-image guard (§3.1); bkgnd-sub mask → sentinel (§3.3); moon window/disclaimer (§4.4); `iss-data-user-guide::1.1` - (§4.2); xml_schema LIDVID style (§4.12); SPICE Time_Coordinates (§5.1); - `__pycache__` prune (§4.1); small template/text items (§5). + (§4.2); xml_schema LIDVID style (§4.12); ~~SPICE Time_Coordinates (§5.1)~~ + (done in `c1168cb`); `__pycache__` prune (§4.1); small template/text + items (§5). 3. **Decisions:** `data_calibrated` forward reference (§4.3); source-product VIDs for v2–v9 images (§4.5); stars-as-targets and 'R'-obsid completeness; cassini: dead-key fields (§5.14); `iss_199rf_fmovie002_prime` navigation @@ -495,7 +512,7 @@ harmless to current data: all 305 production backgrounds are degree 1). obsids (reprojection does NOT need rerunning), so mosaic times become float64. 5. **Guide edits:** §4.7–4.10 concept fixes + §5 minors (on top of the - branch's 1a85209). + already-merged 1a85209). 6. **Regenerate the bundle**; verify ERRORS.log empty; then **re-capture the guide's §4 excerpts and counts from this final bundle**, rebuild the PDF, and re-run (or re-copy) the document collection. From e4ea629ecb87934923ca06ebc88d5a04b1b1cbdd Mon Sep 17 00:00:00 2001 From: Robert French Date: Tue, 11 Aug 2026 16:07:28 -0700 Subject: [PATCH 3/9] critiques: record the float64 time fix merge and data rebuild rms-csmithing fix_mosaic_time_float64 is merged (365621c) and all 305 mosaics and backgrounds have been rebuilt. Verified in the new metadata that time is now float64 with the true per-image cadence (533.746/538.247 s) rather than the float32-quantized 528/544 s steps in the shipped bundle, so blocker B2 is resolved in the data and only the bundle regeneration remains. Also re-verified section 3.3 against the freshly rebuilt backgrounds: the masked-but-not-sentinel pixels are still present in 299 of 305 files (236,915 pixels), confirming the data rebuild does not address it because the defect is in how the generator reads the npz mask. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-08-10.md | 49 +++++++++++++------ 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md index d97736a..79ddcfc 100644 --- a/critiques/pds4-bundle-gen-critique-2026-08-10.md +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -50,13 +50,17 @@ against pre-fix data.** guide corrections (1a85209). **None of it is in this bundle**, which was generated from `9612b2e`; a regeneration is required for any of it to appear in the archive. -2. **The rms-csmithing float64 time fix is still not merged and the data was - never rebuilt.** `/seti/nav/rms-csmithing` has `fix_mosaic_time_float64` - (f156fdb) pushed and open as PR #5, but `origin/main` is still at - `460488f`, and every mosaic/bkgnd file under - `/data/cb-results/fring/ring_mosaic/` is dated **Jul 23** — before the fix. - The bundle therefore inherits float32-quantized times (§3.2). - *(Status re-verified 2026-08-11.)* +2. **The rms-csmithing float64 time fix was not merged and the data was not + rebuilt when this bundle was built.** Every mosaic/bkgnd file under + `/data/cb-results/fring/ring_mosaic/` was dated **Jul 23**, before the fix, + so the bundle inherits float32-quantized times (§3.2). + *(Update 2026-08-11: `fix_mosaic_time_float64` merged as rms-csmithing + `365621c`, and all 305 mosaics and backgrounds rebuilt. Verified in the new + data: `metadata['time']` is now `float64`, values are no longer + float32-representable, and consecutive per-image times differ by + 533.746/538.247 s — the true cadence — instead of the quantized 528/544 s + steps seen in the shipped bundle. The archive still carries the old values + until it is regenerated.)* 3. Working tree: only the user's uncommitted `pds4_bundle_gen/TODO.txt` edit (which now also contains a pasted copy of today's crash traceback). 4. Generation logs for today's run: 1 uncaught exception (§3.1) and 73 @@ -76,7 +80,7 @@ In recommended fix order (details in the cited sections): | # | Blocker | Where fixed | |---|---------|-------------| | B1 | ~~`code_review_fixes` unmerged~~ → **merged 2026-08-11 (`9fc7045`)**; all known label-validation errors are still present in *this build*, which predates the merge, so a regeneration is required | done — regenerate (§6) | -| B2 | float32-quantized `rings:observed_event_tdb` in all mosaic params tables (±8–32 s → up to ~0.2° derived-longitude error) | merge rms-csmithing fix, **rebuild mosaics + bkgnd** (not reproject), regenerate (§3.2) | +| B2 | float32-quantized `rings:observed_event_tdb` in all mosaic params tables (±8–32 s → up to ~0.2° derived-longitude error) | ~~merge rms-csmithing fix~~, ~~rebuild mosaics + bkgnd~~ — **both done 2026-08-11 and verified**; regenerate the bundle to pick them up (§3.2) | | B3 | 299/305 bkgnd-sub mosaics archive masked/bad pixels as valid-looking I/F instead of −999 | new generator (or bkgnd-writer) fix (§3.3) | | B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | new generator fix (duplicate-keyword tolerance) + regenerate obsid (§3.1) | | B5 | `__pycache__` with 5 `.pyc` files inside `document/user_guide/` | delete; prune in packaging (§4.1) | @@ -163,12 +167,20 @@ quantized ETs**, so those columns inherit errors up to ~0.2° / ~1 km. Reprojected-image tables are unaffected (per-image scalar float64 midtime; verified <0.01° round-trip). -The fix (`rms-csmithing` `fix_mosaic_time_float64`, f156fdb) exists but the -mosaic (`ring_ui_mosaic.py`) and background (`ring_ui_bkgnd.py`) stages have +The fix (`rms-csmithing` `fix_mosaic_time_float64`, f156fdb) existed but the +mosaic (`ring_ui_mosaic.py`) and background (`ring_ui_bkgnd.py`) stages had not been rerun since (all data files Jul 23). **Reprojection does not need to be rerun.** Required order: merge the fix → rebuild mosaics → rebuild backgrounds → regenerate the bundle. +**RESOLVED in the data 2026-08-11** (fix merged as rms-csmithing `365621c`; +all 305 mosaics and backgrounds rebuilt). Verified in the new metadata: +`time` is `float64`, the values are no longer float32-representable, and for +`ISS_029RF_FMOVIE001_VIMS` consecutive per-image times now differ by +533.746/538.247 s versus the 528/544 s quantized steps in the shipped bundle — +a shift of several seconds per column, as predicted. The archive itself still +carries the old quantized values until the bundle is regenerated. + ### 3.3 Background-subtracted mosaics archive masked "bad" pixels as valid I/F — NEW (verified; resolves TODO line 64 in the negative) `generate_pds4_files.py:800–806` discards the npz mask @@ -189,6 +201,11 @@ time (simplest, no data rebuild needed beyond §3.2's), and/or sentinel them in the bkgnd writer. The photometry pipeline reads the mask via `f_ring_util` and is unaffected; only the archive is wrong. +*(Re-verified 2026-08-11 against the freshly rebuilt backgrounds: still +**299 of 305** files, 236,915 masked-but-not-sentinel pixels in total. The +data rebuild does not address this — the defect is in how the generator reads +the mask, so it must be fixed in code before regenerating.)* + --- ## 4. Major findings @@ -495,9 +512,8 @@ scripts simplified, SPICE `Time_Coordinates` removed — which also resolves ## 9. Recommended path to a final bundle -1. ~~**Merge** `code_review_fixes` into main~~ — **done 2026-08-11** - (`9fc7045`). Still to do: merge rms-csmithing `fix_mosaic_time_float64` - (PR #5) into its main. +1. ~~**Merge** `code_review_fixes` into main (`9fc7045`); merge rms-csmithing + `fix_mosaic_time_float64` (`365621c`)~~ — **both done 2026-08-11.** 2. **New code fixes** (this critique): duplicate-keyword-tolerant PDS3 lookup + `KeyError` backstop + stronger per-image guard (§3.1); bkgnd-sub mask → sentinel (§3.3); moon window/disclaimer (§4.4); `iss-data-user-guide::1.1` @@ -508,9 +524,12 @@ scripts simplified, SPICE `Time_Coordinates` removed — which also resolves VIDs for v2–v9 images (§4.5); stars-as-targets and 'R'-obsid completeness; cassini: dead-key fields (§5.14); `iss_199rf_fmovie002_prime` navigation check. -4. **Rebuild data:** `ring_ui_mosaic.py` then `ring_ui_bkgnd.py` for all 305 +4. ~~**Rebuild data:** `ring_ui_mosaic.py` then `ring_ui_bkgnd.py` for all 305 obsids (reprojection does NOT need rerunning), so mosaic times become - float64. + float64.~~ — **done 2026-08-11**, all 305 rebuilt and the float64 times + verified. Note this rebuild does **not** fix §3.3: the masked-pixel + sentinel problem is in how the bundle generator reads the npz mask, so it + persists in the rebuilt background-subtracted mosaics. 5. **Guide edits:** §4.7–4.10 concept fixes + §5 minors (on top of the already-merged 1a85209). 6. **Regenerate the bundle**; verify ERRORS.log empty; then **re-capture the From 7b67f1fda0bd21cdd182a0b235a444bf15984c6b Mon Sep 17 00:00:00 2001 From: Robert French Date: Tue, 11 Aug 2026 20:01:03 -0700 Subject: [PATCH 4/9] critiques: withdraw the background mask finding, record the 287RI fix Section 3.3 was wrong. The mask stored with a background-subtracted mosaic marks the pixels that were excluded when fitting the background gradient (stars, moons, and the like). Those are real data, so discarding the mask is correct and sentinelling them would have destroyed valid science data. The requirement that does matter is that pixels missing from the original mosaic are sentinels rather than merely masked, and that was checked directly against all 305 mosaic/background pairs: every -999 pixel in a mosaic is also -999 in the background-subtracted data, with no exceptions. ring_ui_bkgnd guarantees this when it builds the corrected mosaic. TODO line 64 is therefore satisfied, not failing as the review claimed. Also record that the duplicate-keyword crash is fixed: reading labels with first_suffix=False lets ISS_287RI_PROPRETRG001_PRIME generate all 19 reprojected products with a clean inventory, so only the exception backstop and the per-image guard remain open in section 3.1. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-08-10.md | 95 ++++++++++--------- 1 file changed, 51 insertions(+), 44 deletions(-) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md index 79ddcfc..98c73c3 100644 --- a/critiques/pds4-bundle-gen-critique-2026-08-10.md +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -22,7 +22,7 @@ generation: the generated bundle at `pds4_bundle_gen/bundle` (→ (listings, field tables, counts, verbatim excerpts, example scripts) re-executed or re-derived against the actual bundle. - Key claims independently re-verified by the orchestrator (float32 ET - quantization, bkgnd-sub mask discard, phantom inventory row). + quantization, background mask handling, phantom inventory row). **Supersedes** the 2026-07-19 and 2026-07-21 critiques (moved to `critiques/archive/`) and complements `critiques/code-review-2026-08-04.md`, @@ -81,8 +81,8 @@ In recommended fix order (details in the cited sections): |---|---------|-------------| | B1 | ~~`code_review_fixes` unmerged~~ → **merged 2026-08-11 (`9fc7045`)**; all known label-validation errors are still present in *this build*, which predates the merge, so a regeneration is required | done — regenerate (§6) | | B2 | float32-quantized `rings:observed_event_tdb` in all mosaic params tables (±8–32 s → up to ~0.2° derived-longitude error) | ~~merge rms-csmithing fix~~, ~~rebuild mosaics + bkgnd~~ — **both done 2026-08-11 and verified**; regenerate the bundle to pick them up (§3.2) | -| B3 | 299/305 bkgnd-sub mosaics archive masked/bad pixels as valid-looking I/F instead of −999 | new generator (or bkgnd-writer) fix (§3.3) | -| B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | new generator fix (duplicate-keyword tolerance) + regenerate obsid (§3.1) | +| ~~B3~~ | ~~bkgnd-sub mosaics archive masked pixels as valid I/F~~ — **withdrawn, not a bug** (§3.3): the mask marks gradient-fit exclusions, which are real data | none | +| B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | ~~duplicate-keyword tolerance~~ **fixed 2026-08-11 and verified (all 19 products, clean inventory)**; regenerate to clear it from the archive (§3.1) | | B5 | `__pycache__` with 5 `.pyc` files inside `document/user_guide/` | delete; prune in packaging (§4.1) | | B6 | Dangling external LIDVID `iss-data-user-guide::1.0` (only `::1.1` exists) | one-character template/CSV fix (§4.2) | | B7 | Guide: missing core concepts (array axis direction, inertial-longitude definition, bkgnd-limit semantics, IMGID convention) | guide edits (§4.7–4.10; §4.6 withdrawn) | @@ -141,17 +141,25 @@ and `pyparsing.ParseException`, so the `KeyError` escapes to the top-level bare *mosaic* inventory rows even though the mosaic products were written — the inverse validate failure (orphan products). Required fix: -1. Duplicate-tolerant PDS3 lookup: if `key` missing, fall back to `f'{key}_1'` - (log a warning; optionally assert `_1 == _2`). Fixing only the SCLK line is - insufficient — `START_TIME`, `STOP_TIME`, `SEQUENCE_TITLE`, `SHUTTER_*`, - `TARGET_DESC`, `TELEMETRY_FORMAT_ID`, `SOFTWARE_VERSION_ID` are read a few - lines later and are all suffixed in this label. +1. ~~Duplicate-tolerant PDS3 lookup~~ — **FIXED 2026-08-11** by reading the + label with `Pds3Label(..., first_suffix=False)`, which keeps the first + occurrence of a duplicated keyword under its plain name. This covers the + whole duplicated block at once, which matters because `START_TIME`, + `STOP_TIME`, `SEQUENCE_TITLE`, `TARGET_DESC`, `TELEMETRY_FORMAT_ID`, and + `SOFTWARE_VERSION_ID` are read a few lines later and were all suffixed in + this label. Verified: all eight affected keys now resolve, the `_1`/`_2` + values agree, normal labels are unaffected, and regenerating + `ISS_287RI_PROPRETRG001_PRIME` produces all **19** reprojected products + (was 13) with 19 inventory rows, 19 products on disk, and zero dangling or + orphan entries. 2. Backstop: convert unexpected exceptions in `xml_add_pds3_label_info` (at - minimum `KeyError`) into `ObsIdFailedException`. -3. On the branch: make the per-image guard survive unexpected exceptions - without aborting the obsid's inventory bookkeeping. + minimum `KeyError`) into `ObsIdFailedException`. **Still to do** — the + duplicate-keyword case is fixed, but any other unexpected exception would + still abort an obsid mid-stream. +3. Make the per-image guard survive unexpected exceptions without aborting the + obsid's inventory bookkeeping. **Still to do.** 4. Regenerate the obsid (products, both collection CSVs, all three global - indexes). + indexes) — covered by the full regeneration. ### 3.2 float32-quantized times contaminate every mosaic params table — data rebuild required (verified numerically) @@ -181,30 +189,29 @@ all 305 mosaics and backgrounds rebuilt). Verified in the new metadata: a shift of several seconds per column, as predicted. The archive itself still carries the old quantized values until the bundle is regenerated. -### 3.3 Background-subtracted mosaics archive masked "bad" pixels as valid I/F — NEW (verified; resolves TODO line 64 in the negative) - -`generate_pds4_files.py:800–806` discards the npz mask -(`metadata['img'].mask = False`) on the assumption that all missing data was -already converted to −999. That assumption is false: in **299 of 305** -production `*-BKGND-SUB-MOSAIC.npz` files there are masked pixels whose stored -value is *not* −999 — e.g. `ISS_007RI_AZSCNLOPH001`: 6,019 of 11,633 masked -pixels carry values −0.0006…0.144 (plausible I/F; independently re-verified). -Mechanism (`mosaics/ring_ui_bkgnd.py:148–153`): `corrected = mosaic_img − -bkgnd_model.data` uses the raw `.data` of the masked background model -(garbage/unfit values at masked pixels); −999 is applied only where the -original pixel was −999 or the entire column is masked. Partially-masked -columns keep unsubtracted/garbage values, which the bundle then archives as -valid data — undetectable by users via `Special_Constants`. - -Fix options: honor the mask in `read_mosaic` (masked → SENTINEL) at bundle -time (simplest, no data rebuild needed beyond §3.2's), and/or sentinel them in -the bkgnd writer. The photometry pipeline reads the mask via `f_ring_util` and -is unaffected; only the archive is wrong. - -*(Re-verified 2026-08-11 against the freshly rebuilt backgrounds: still -**299 of 305** files, 236,915 masked-but-not-sentinel pixels in total. The -data rebuild does not address this — the defect is in how the generator reads -the mask, so it must be fixed in code before regenerating.)* +### 3.3 NOT A BUG (withdrawn) — background-subtracted mosaics and the npz mask + +The original finding claimed that `generate_pds4_files.py` wrongly discards the +background-subtracted mosaic's mask (`metadata['img'].mask = False`), archiving +"bad" pixels as valid I/F. **That was a misreading of what the mask means.** + +Per user confirmation, the mask marks only the pixels that were excluded when +*fitting the background gradient* — stars, moons, and similar contaminants. +Those pixels are real data and belong in the archive; discarding the mask is +the correct behavior, and sentinelling them would destroy valid science data. + +The requirement that does matter — **pixels missing from the original mosaic +must be sentinels, not merely masked** — was checked directly and **holds**: +across all 305 mosaic/background-subtracted pairs, every pixel that is −999 in +the original mosaic is also −999 in the background-subtracted data, with zero +exceptions. `mosaics/ring_ui_bkgnd.py:150` guarantees this +(`corrected[mosaic_img == -999] = -999`), and line 152 additionally sentinels +whole columns whose background model is entirely masked. Mosaics represent +missing data as −999 (254 of 305 files contain it; the other 51 have complete +coverage), so nothing is lost at the reprojection-limited mosaic edges. + +TODO line 64 ("verify that the sentinel values are present in the background +mosaics from the original mosaic's bad pixels") is therefore **satisfied**. --- @@ -358,7 +365,7 @@ already does) or confirm exact LIDVIDs with EN/RMS. `data_reproj_img.lblx:329–333` describes only reproj semantics ("off the edge of the FOV / transmission error"); for mosaics the dominant meaning is "no image covered this longitude", and for bkg_sub also "background model - invalid" (esp. once §3.3 is fixed). + invalid". 7. `spacecraft_clock_count_partition` hardcoded `1` in both data templates while the generator parses the real `SPACECRAFT_CLOCK_CNT_PARTITION` (dead key). Always 1 for Cassini; still, use the variable. @@ -471,7 +478,7 @@ scripts simplified, SPICE `Time_Coordinates` removed — which also resolves | Limit cameras/targets to those present | Largely satisfied (labels are per-camera; collections genuinely contain both moons/cameras). Optional. | | Cassini field diffs vs original bundle | Content decisions; two "FROM WHERE?" answers found — see §5 item 14. | | `rings:description` update | Blocked on external dictionary cleanup; defer. | -| Verify sentinels in bkgnd mosaics | **Done — FAILS (§3.3). Must fix.** | +| Verify sentinels in bkgnd mosaics | **Done — PASSES** (§3.3): all 305 pairs check out. | | Wrap-around limits / example labels / bundle.lblx / moons | Marked DONE; consistent with code. | --- @@ -514,9 +521,11 @@ scripts simplified, SPICE `Time_Coordinates` removed — which also resolves 1. ~~**Merge** `code_review_fixes` into main (`9fc7045`); merge rms-csmithing `fix_mosaic_time_float64` (`365621c`)~~ — **both done 2026-08-11.** -2. **New code fixes** (this critique): duplicate-keyword-tolerant PDS3 lookup + - `KeyError` backstop + stronger per-image guard (§3.1); bkgnd-sub mask → - sentinel (§3.3); moon window/disclaimer (§4.4); `iss-data-user-guide::1.1` +2. **New code fixes** (this critique): ~~duplicate-keyword-tolerant PDS3 + lookup (§3.1)~~ (done — `Pds3Label(..., first_suffix=False)`, verified: the + 287RI obsid now produces all 19 reproj products with no dangling or orphan + inventory rows); still to do: `KeyError` backstop + stronger per-image + guard (§3.1); moon window/disclaimer (§4.4); `iss-data-user-guide::1.1` (§4.2); xml_schema LIDVID style (§4.12); ~~SPICE Time_Coordinates (§5.1)~~ (done in `c1168cb`); `__pycache__` prune (§4.1); small template/text items (§5). @@ -527,9 +536,7 @@ scripts simplified, SPICE `Time_Coordinates` removed — which also resolves 4. ~~**Rebuild data:** `ring_ui_mosaic.py` then `ring_ui_bkgnd.py` for all 305 obsids (reprojection does NOT need rerunning), so mosaic times become float64.~~ — **done 2026-08-11**, all 305 rebuilt and the float64 times - verified. Note this rebuild does **not** fix §3.3: the masked-pixel - sentinel problem is in how the bundle generator reads the npz mask, so it - persists in the rebuilt background-subtracted mosaics. + verified. 5. **Guide edits:** §4.7–4.10 concept fixes + §5 minors (on top of the already-merged 1a85209). 6. **Regenerate the bundle**; verify ERRORS.log empty; then **re-capture the From 871bebd7d7717663bdd775878243b58fd3fe6a90 Mon Sep 17 00:00:00 2001 From: Robert French Date: Tue, 11 Aug 2026 20:37:10 -0700 Subject: [PATCH 5/9] critiques: correct the __pycache__ finding and close out section 3.1 The __pycache__ directory in document/user_guide was created by this review running the shipped example scripts in place inside the bundle, not by the generator; its mtime is hours after generation finished. The user has deleted it, so it needs no code change, only the procedural note to run those scripts from a copy. Also record that the remaining section 3.1 items are done: the label reader now logs an error and raises ObsIdFailedException on any unexpected failure, the per-image loop drops only the affected image, and a sys.excepthook logs fatal failures that happen outside the per-OBSID loop. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-08-10.md | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md index 98c73c3..327b9b7 100644 --- a/critiques/pds4-bundle-gen-critique-2026-08-10.md +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -83,7 +83,7 @@ In recommended fix order (details in the cited sections): | B2 | float32-quantized `rings:observed_event_tdb` in all mosaic params tables (±8–32 s → up to ~0.2° derived-longitude error) | ~~merge rms-csmithing fix~~, ~~rebuild mosaics + bkgnd~~ — **both done 2026-08-11 and verified**; regenerate the bundle to pick them up (§3.2) | | ~~B3~~ | ~~bkgnd-sub mosaics archive masked pixels as valid I/F~~ — **withdrawn, not a bug** (§3.3): the mask marks gradient-fit exclusions, which are real data | none | | B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | ~~duplicate-keyword tolerance~~ **fixed 2026-08-11 and verified (all 19 products, clean inventory)**; regenerate to clear it from the archive (§3.1) | -| B5 | `__pycache__` with 5 `.pyc` files inside `document/user_guide/` | delete; prune in packaging (§4.1) | +| ~~B5~~ | ~~`__pycache__` with 5 `.pyc` files inside `document/user_guide/`~~ — **resolved**; it was created by this review running the example scripts in place, not by the generator, and the user has deleted it (§4.1) | none | | B6 | Dangling external LIDVID `iss-data-user-guide::1.0` (only `::1.1` exists) | one-character template/CSV fix (§4.2) | | B7 | Guide: missing core concepts (array axis direction, inertial-longitude definition, bkgnd-limit semantics, IMGID convention) | guide edits (§4.7–4.10; §4.6 withdrawn) | | B8 | Guide: every §4 verbatim numeric excerpt and the product counts disagree with the real bundle | re-capture from the **final** bundle, rebuild PDF (§4.11) | @@ -152,12 +152,16 @@ inverse validate failure (orphan products). Required fix: `ISS_287RI_PROPRETRG001_PRIME` produces all **19** reprojected products (was 13) with 19 inventory rows, 19 products on disk, and zero dangling or orphan entries. -2. Backstop: convert unexpected exceptions in `xml_add_pds3_label_info` (at - minimum `KeyError`) into `ObsIdFailedException`. **Still to do** — the - duplicate-keyword case is fixed, but any other unexpected exception would - still abort an obsid mid-stream. -3. Make the per-image guard survive unexpected exceptions without aborting the - obsid's inventory bookkeeping. **Still to do.** +2. ~~Backstop: convert unexpected exceptions in `xml_add_pds3_label_info` into + `ObsIdFailedException`~~ — **FIXED 2026-08-11**; it now logs an error naming + the label file first. +3. ~~Make the per-image guard survive unexpected exceptions without aborting + the obsid's inventory bookkeeping~~ — **FIXED 2026-08-11**. A + `sys.excepthook` was also added so that a failure anywhere outside the + per-OBSID loop is logged as an error instead of dying silently. + Fault-injection verified: one forced exception yields one logged error, 18 + of 19 products, and consistent inventories with no dangling or orphan + rows. 4. Regenerate the obsid (products, both collection CSVs, all three global indexes) — covered by the full regeneration. @@ -217,12 +221,16 @@ mosaics from the original mosaic's bad pixels") is therefore **satisfied**. ## 4. Major findings -### 4.1 `__pycache__` inside the document collection — NEW -`document/user_guide/__pycache__/` contains five `.cpython-312.pyc` files -(mtime 20:19, i.e. created *after* generation by running the example scripts in -place). Unlabeled, non-archival → `validate` errors. Delete before delivery and -make the generator/packaging prune `__pycache__` (and never execute the shipped -scripts inside the bundle tree). +### 4.1 RESOLVED, self-inflicted — `__pycache__` inside the document collection +`document/user_guide/__pycache__/` held five `.cpython-312.pyc` files. **This +was an artifact of this review, not of the generator**: the mtime (20:19) is +hours after generation finished (16:06), and it matches the point at which +the review agents ran the shipped example scripts in place inside the bundle +to check that they work. The generator never creates these files. + +**Deleted by the user 2026-08-11.** No code change is needed. The only lasting +lesson is procedural: run the shipped example scripts from a copy, never from +inside the bundle tree, and check for stray files before delivery. ### 4.2 Dangling external LIDVID: `iss-data-user-guide::1.0` — NEW `document/collection_document.csv:2` and @@ -522,13 +530,12 @@ scripts simplified, SPICE `Time_Coordinates` removed — which also resolves 1. ~~**Merge** `code_review_fixes` into main (`9fc7045`); merge rms-csmithing `fix_mosaic_time_float64` (`365621c`)~~ — **both done 2026-08-11.** 2. **New code fixes** (this critique): ~~duplicate-keyword-tolerant PDS3 - lookup (§3.1)~~ (done — `Pds3Label(..., first_suffix=False)`, verified: the - 287RI obsid now produces all 19 reproj products with no dangling or orphan - inventory rows); still to do: `KeyError` backstop + stronger per-image - guard (§3.1); moon window/disclaimer (§4.4); `iss-data-user-guide::1.1` - (§4.2); xml_schema LIDVID style (§4.12); ~~SPICE Time_Coordinates (§5.1)~~ - (done in `c1168cb`); `__pycache__` prune (§4.1); small template/text - items (§5). + lookup, `KeyError` backstop, and a stronger per-image + guard (§3.1)~~ (all done — see §3.1); still to do: moon window/disclaimer + (§4.4); `iss-data-user-guide::1.1` (§4.2); xml_schema LIDVID style + (§4.12); ~~SPICE Time_Coordinates (§5.1)~~ (done in `c1168cb`); + ~~`__pycache__` prune (§4.1)~~ (not a generator issue); small + template/text items (§5). 3. **Decisions:** `data_calibrated` forward reference (§4.3); source-product VIDs for v2–v9 images (§4.5); stars-as-targets and 'R'-obsid completeness; cassini: dead-key fields (§5.14); `iss_199rf_fmovie002_prime` navigation From c4461f62f72966a2fc203855f1805925674f4949 Mon Sep 17 00:00:00 2001 From: Robert French Date: Tue, 11 Aug 2026 20:58:42 -0700 Subject: [PATCH 6/9] critiques: record the satellite target reconciliation fix Section 4.4 is fixed in 160af03: a 50 km radial tolerance when the observation log confirms a moon by eye, a wrap-aware edge check, warnings that state why a confirmed moon was rejected, and label text that no longer contradicts the log. Warnings across all 305 observations drop from 73 to 63, and the remaining ten are itemized with their measured misses. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-08-10.md | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md index 327b9b7..b33f3fb 100644 --- a/critiques/pds4-bundle-gen-critique-2026-08-10.md +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -87,7 +87,7 @@ In recommended fix order (details in the cited sections): | B6 | Dangling external LIDVID `iss-data-user-guide::1.0` (only `::1.1` exists) | one-character template/CSV fix (§4.2) | | B7 | Guide: missing core concepts (array axis direction, inertial-longitude definition, bkgnd-limit semantics, IMGID convention) | guide edits (§4.7–4.10; §4.6 withdrawn) | | B8 | Guide: every §4 verbatim numeric excerpt and the product counts disagree with the real bundle | re-capture from the **final** bundle, rebuild PDF (§4.11) | -| B9 | Moon `Target_Identification` wrong for the 12+8 "visually confirmed but geometrically rejected" mosaics; "not visually confirmed" disclaimer emitted even when confirmed | new generator fix / policy decision (§4.4) | +| ~~B9~~ | ~~Moon `Target_Identification` wrong for the "visually confirmed but geometrically rejected" mosaics; disclaimer emitted even when confirmed~~ — **fixed 2026-08-11 (`160af03`)**; 50 km tolerance, wrap-aware edge check, conditional label text; warnings 73 → 63, all remaining genuine | done (§4.4) | | B10 | Decisions required: `data_calibrated` forward references (§4.3), source-product VIDs `::1.0` vs PDS3 versions 2–9 (§4.5), xml_schema LIDVIDs (§4.12), open TODO items (§7) | user | --- @@ -271,10 +271,30 @@ False + 12 geo-False/vis-True; Pandora 7 + 8. - Latent: the edge check (:922) uses compressed-array indices with no wraparound — a moon near corot 0° in a full-360° mosaic is wrongly rejected. -Suggested: widen the window by moon radius + model tolerance (or let the -visual flag override for mosaics), and condition the disclaimer sentence on -the flag. (Commit 83b46eb's inertial-longitude core radius was verified -correct; residual error ~0.05 km.) +**FIXED 2026-08-11** (`160af03`). A moon that is geometrically present but not +visually confirmed is still listed, with a warning, as before. A moon that is +visually confirmed but geometrically absent is now accepted when it is within +**50 km** of the radial limits — inside the error budget of an orbit model good +to a few tens of km for satellites tens of km across. Otherwise it is dropped +and the warning states the reason, distinguishing a radial miss from a +longitude with no valid data. Both decisions are computed from each product's own valid +longitudes, so a mosaic and its background-subtracted version are evaluated +separately. The edge check now treats the ends of the valid-longitude array as +adjacent when the coverage wraps through 0/360, and requires at least five +valid longitudes. Mosaic labels no longer assert "has not been visually +confirmed" when the log says it was; reprojected-image labels keep the original +wording since they carry no per-image visual check. + +Measured over all 305 observations: warnings drop from 73 to **63**. The 10 +remaining are genuine — 6 are 73–277 km beyond the radial limit +(`ISS_082RI_FMONITOR003_PRIME` 276.5 km, `ISS_207RF_FMOVIE001_PRIME` 159.0 km, +`ISS_087RF_FMOVIE003_PRIME` 73.4 km, each in both variants) and 4 sit at +longitudes with no valid data in the background-subtracted mosaic +(`ISS_007RI_LPHRLFMOV001_PRIME` 0.320°, `ISS_172RI_SPOKEMOV001_PRIME` 0.148°, +`ISS_105RI_TMAPN45LP001_CIRS_6` 0.070°, `ISS_105RI_TMAPN45LP001_CIRS_4` +0.044°). The last of those is a near-miss against the 2-bin (0.04°) longitude +tolerance and may be worth a second look. (Commit 83b46eb's inertial-longitude +core radius was verified correct; residual error ~0.05 km.) ### 4.5 `Source_Product_Internal` hardcodes `::1.0` while 165 source images are PDS3 versions 2–9 — VERIFY EXTERNALLY `image_name_to_calib_lidvid` (:1026–1033) always emits `::1.0`. The mosaics use From 85338e5939a34f8ff7600b5dbd072a3a71b6f6e2 Mon Sep 17 00:00:00 2001 From: Robert French Date: Wed, 12 Aug 2026 19:23:32 -0700 Subject: [PATCH 7/9] critiques: record the resolved external reference questions The Cassini ISS user's guide is version 2.0, not 1.0, and the calibrated collection is urn:nasa:pds:cassini_iss_saturn:data_calibrated::1.0, both per the user. Section 4.2 is fixed in 56ce1d3, and sections 4.3 and 4.5 are resolved: the per-image source-product references are deliberate forward references to a coordinated ISS delivery, and ::1.0 is correct for every image because the PDS4 product LID is keyed on the spacecraft clock count rather than the PDS3 file version. Both references point at products that are not in the published holdings yet, so validate will flag them until that delivery lands. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- .../pds4-bundle-gen-critique-2026-08-10.md | 57 +++++++++++-------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/pds4-bundle-gen-critique-2026-08-10.md index b33f3fb..67afc19 100644 --- a/critiques/pds4-bundle-gen-critique-2026-08-10.md +++ b/critiques/pds4-bundle-gen-critique-2026-08-10.md @@ -84,11 +84,11 @@ In recommended fix order (details in the cited sections): | ~~B3~~ | ~~bkgnd-sub mosaics archive masked pixels as valid I/F~~ — **withdrawn, not a bug** (§3.3): the mask marks gradient-fit exclusions, which are real data | none | | B4 | `ISS_287RI_PROPRETRG001_PRIME` incomplete: 6 reproj products missing, 1 phantom inventory row ×2 collections, 6 dangling src_imgs LIDVIDs ×2 tables | ~~duplicate-keyword tolerance~~ **fixed 2026-08-11 and verified (all 19 products, clean inventory)**; regenerate to clear it from the archive (§3.1) | | ~~B5~~ | ~~`__pycache__` with 5 `.pyc` files inside `document/user_guide/`~~ — **resolved**; it was created by this review running the example scripts in place, not by the generator, and the user has deleted it (§4.1) | none | -| B6 | Dangling external LIDVID `iss-data-user-guide::1.0` (only `::1.1` exists) | one-character template/CSV fix (§4.2) | +| ~~B6~~ | ~~Dangling external LIDVID `iss-data-user-guide::1.0`~~ — **fixed 2026-08-11 (`56ce1d3`)**: the correct version is `::2.0` (user-supplied); forward reference until the new ISS delivery is published (§4.2) | done | | B7 | Guide: missing core concepts (array axis direction, inertial-longitude definition, bkgnd-limit semantics, IMGID convention) | guide edits (§4.7–4.10; §4.6 withdrawn) | | B8 | Guide: every §4 verbatim numeric excerpt and the product counts disagree with the real bundle | re-capture from the **final** bundle, rebuild PDF (§4.11) | | ~~B9~~ | ~~Moon `Target_Identification` wrong for the "visually confirmed but geometrically rejected" mosaics; disclaimer emitted even when confirmed~~ — **fixed 2026-08-11 (`160af03`)**; 50 km tolerance, wrap-aware edge check, conditional label text; warnings 73 → 63, all remaining genuine | done (§4.4) | -| B10 | Decisions required: `data_calibrated` forward references (§4.3), source-product VIDs `::1.0` vs PDS3 versions 2–9 (§4.5), xml_schema LIDVIDs (§4.12), open TODO items (§7) | user | +| B10 | Decisions: ~~`data_calibrated` forward references (§4.3)~~ and ~~source-product VIDs (§4.5)~~ **both resolved 2026-08-11**; still open: xml_schema LIDVIDs (§4.12), open TODO items (§7) | user | --- @@ -232,23 +232,29 @@ to check that they work. The generator never creates these files. lesson is procedural: run the shipped example scripts from a copy, never from inside the bundle tree, and check for stray files before delivery. -### 4.2 Dangling external LIDVID: `iss-data-user-guide::1.0` — NEW -`document/collection_document.csv:2` and -`miscellaneous/collection_miscellaneous.csv:4` (sources: -`templates/collection_document.csv`, `templates/collection_miscellaneous.csv`) -reference `urn:nasa:pds:cassini_iss_saturn:document:iss-data-user-guide::1.0`. -The PDS registry and the live RMS inventory have only **`::1.1`**; `::1.0` -does not exist → referential-integrity error at ingestion. Change to `::1.1` -(or drop the VID). `bundle.lblx` is safe (LID-only reference). +### 4.2 FIXED — dangling external LIDVID `iss-data-user-guide::1.0` +`templates/collection_document.csv:2` and +`templates/collection_miscellaneous.csv:4` referenced +`urn:nasa:pds:cassini_iss_saturn:document:iss-data-user-guide::1.0`, which does +not exist. `bundle.lblx` was never affected (LID-only reference). -### 4.3 All 20,435 reproj labels reference `cassini_iss_saturn:data_calibrated` — DECISION NEEDED +**Fixed 2026-08-11 (`56ce1d3`)**: the correct version is **`::2.0`** +(user-supplied). Note that the published RMS holdings currently list `::1.1`, +so like §4.3 this is a forward reference that resolves when the new ISS +delivery is published — `validate` will flag it against today's holdings. + +### 4.3 RESOLVED — reproj labels reference `cassini_iss_saturn:data_calibrated` Every reproj label's `Source_Product_Internal` points at -`urn:nasa:pds:cassini_iss_saturn:data_calibrated:_calib::1.0`, but the -archived `cassini_iss_saturn::1.1` bundle contains no `data_calibrated` -collection (only browse_raw/context/data_raw/document/xml_schema; registry -lookups return not-found). If a calibrated-ISS delivery is coordinated with -RMS this is a deliberate forward reference; otherwise every reproj label ships -a dangling source-product LIDVID. Confirm with RMS before final. +`urn:nasa:pds:cassini_iss_saturn:data_calibrated:_calib::1.0`, which the +published `cassini_iss_saturn::1.1` bundle does not yet contain (it holds only +browse_raw/context/data_raw/document/xml_schema, and registry lookups return +not-found). + +**Confirmed correct by the user 2026-08-11**: the calibrated collection is +`urn:nasa:pds:cassini_iss_saturn:data_calibrated::1.0`. These are deliberate +forward references to a coordinated ISS delivery, not dangling LIDVIDs. They +will not resolve against the currently published holdings, so expect +`validate` to flag them until that delivery lands. ### 4.4 Moon `Target_Identification` policy: geometric test overrides the human "visually confirmed" flags — NEW `generate_pds4_files.py:1929–1941` warns on disagreement but the geometric @@ -296,13 +302,16 @@ longitudes with no valid data in the background-subtracted mosaic tolerance and may be worth a second look. (Commit 83b46eb's inertial-longitude core radius was verified correct; residual error ~0.05 km.) -### 4.5 `Source_Product_Internal` hardcodes `::1.0` while 165 source images are PDS3 versions 2–9 — VERIFY EXTERNALLY -`image_name_to_calib_lidvid` (:1026–1033) always emits `::1.0`. The mosaics use -165 source images with PDS3 version suffix ≥ 2 (v2:37 … v9:2). If the migrated -calibrated bundle assigns VIDs per PDS3 version (RMS convention), those 165 -reproj labels reference superseded versions. Check against the actual -`cassini_iss_saturn` calibrated delivery (not resolvable from this machine); -combine with the §4.3 decision. +### 4.5 RESOLVED — `Source_Product_Internal` emits `::1.0` for every source image +`image_name_to_calib_lidvid` always emits `::1.0`, while 165 of the source +images carry a PDS3 version suffix of 2 or higher (v2:37 … v9:2). The concern +was that the migrated collection might version products to match. + +**Resolved by §4.3**: the calibrated collection is at `::1.0`, and the PDS4 +product LID (`1880796883n_calib`) is keyed on the spacecraft clock count and +does not carry the PDS3 file version at all, so `::1.0` is right for every +image regardless of its PDS3 version. Worth one confirmation with RMS when the +delivery lands, since a mismatch would affect 165 labels. ### 4.6 NOT A BUG (withdrawn) — guide's "emission < 90° = lit side" statement The original finding assumed a north-based emission convention. Per user From ff80f092337b42cb52a96059620d936026089a6d Mon Sep 17 00:00:00 2001 From: Robert French Date: Wed, 9 Sep 2026 13:23:15 -0700 Subject: [PATCH 8/9] critiques: repoint the two predecessor paths at archive/ Moving the July critiques under critiques/archive/ left two cross-references pointing at paths that no longer exist: code-review-2026-08-04.md naming the 07-21 critique as its predecessor, and 07-21 naming 07-19 as its own. Both now carry the archive/ path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- critiques/archive/pds4-bundle-gen-critique-2026-07-21.md | 2 +- critiques/code-review-2026-08-04.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/critiques/archive/pds4-bundle-gen-critique-2026-07-21.md b/critiques/archive/pds4-bundle-gen-critique-2026-07-21.md index 6dfcae6..73c932a 100644 --- a/critiques/archive/pds4-bundle-gen-critique-2026-07-21.md +++ b/critiques/archive/pds4-bundle-gen-critique-2026-07-21.md @@ -4,7 +4,7 @@ **Software reviewed:** `/seti/research/f-ring/f-ring/pds4_bundle_gen/` — `generate_pds4_files.py` (3,817 lines), `templates/*.lblx`, `templates/examples/*.py`, `templates/*.csv` **Users guide compared against:** `/seti/research/f-ring/f-ring/users_guide/sections/*.tex` (LaTeX sources; `main.pdf` built 2026-07-19, md5 `e6e7eef933…`, shipped identically in the bundle) **Ground truth:** the regenerated bundle at `/data/fring-bundles/pds4/` (rebuilt 2026-07-21 18:23). -**Predecessor:** `critiques/pds4-bundle-gen-critique-2026-07-19.md`. Line numbers refer to the current files. +**Predecessor:** `critiques/archive/pds4-bundle-gen-critique-2026-07-19.md`. Line numbers refer to the current files. --- diff --git a/critiques/code-review-2026-08-04.md b/critiques/code-review-2026-08-04.md index 70d3ba1..7175af5 100644 --- a/critiques/code-review-2026-08-04.md +++ b/critiques/code-review-2026-08-04.md @@ -10,7 +10,7 @@ itself (it is mid-regeneration; a bundle spot-check should be a second pass). consistency of units, masks, and sentinel values; code-vs-guide consistency. **Method:** nine parallel line-by-line reviews by area, followed by independent re-verification of every critical/major claim against the source (and, where marked, numerically or empirically). -**Predecessor:** `critiques/pds4-bundle-gen-critique-2026-07-21.md` (status of its findings in §6). +**Predecessor:** `critiques/archive/pds4-bundle-gen-critique-2026-07-21.md` (status of its findings in §6). > **Fix status (updated 2026-08-04):** every finding in this report was fixed on branch > `code_review_fixes` — commits `52cb449` (mosaics), `115574b` (f_ring_util + photometry), From 39d9775ebf9db4d34d906b924859b4801e101705 Mon Sep 17 00:00:00 2001 From: Robert French Date: Wed, 9 Sep 2026 13:52:03 -0700 Subject: [PATCH 9/9] critiques: archive the 2026-08-10 review The 2026-09-03 critique supersedes it, so it joins the July pair under critiques/archive/. Nothing references it by path -- the 09-03 critique cites it by date -- so the move needs no follow-up edits. critiques/ now holds the 08-04 code review and the 09-03 critique at the top level, with three superseded reviews archived beneath. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013h7F5N8axKM4bV5eNpPVvV --- critiques/{ => archive}/pds4-bundle-gen-critique-2026-08-10.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename critiques/{ => archive}/pds4-bundle-gen-critique-2026-08-10.md (100%) diff --git a/critiques/pds4-bundle-gen-critique-2026-08-10.md b/critiques/archive/pds4-bundle-gen-critique-2026-08-10.md similarity index 100% rename from critiques/pds4-bundle-gen-critique-2026-08-10.md rename to critiques/archive/pds4-bundle-gen-critique-2026-08-10.md