Validate compressed-SLC inputs and fix forward-mode stack-depth count - #378
Merged
mgovorcin merged 8 commits intoAug 18, 2026
Merged
Conversation
…count Adds fail-fast input checks for compressed SLC (CCSLC) date conflicts and corrects the forward-mode network depth requirement, so malformed input stacks are rejected at config time instead of crashing deep inside dolphin. main.py: - New `_assert_no_compressed_slc_conflicts`, run in every processing mode whenever CCSLCs are present (historical accepts them as prior baselines, not just forward). Raises 1001 when a real SLC's date equals its burst's most-recent CCSLC reference date, and 1002 when a CCSLC's reference date reaches past the real SLCs it is paired with. The 1002 boundary is mode-aware: forward outputs a single product so the CCSLC need only predate the latest real date, while historical outputs one product per new date so the CCSLC must predate all of them. Both previously surfaced as a pydantic ValidationError from dolphin's ministack construction -- immediately in forward mode, but only after ~90s of processing (mask extraction, PS pixels, EMI setup) in historical mode. - `_assert_forward_mode_compressed` now covers only the forward-only requirement that every burst has a CCSLC (2000); historical runs with zero CCSLCs are unaffected. - Log the reference date(s) of saved compressed SLCs, not just the count. pge_runconfig.py: - `_create_forward_mode_network` counted compressed SLCs toward the nearest-N depth, but dolphin builds the manual-index network from the real-date phase-linked outputs only (globbed as "2*.tif"; compressed outputs are globbed separately). Count real SLCs only. Undercounting produced an IndexError in dolphin.interferogram._make_ifg_pairs instead of a clear error here. Renumbered 2002 -> 2001 so the code is unique. docs/: compressed-SLC operational logic (phase-linking reference selection, the two interferogram groups, filename conventions) and a reference for all five input-validation error codes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 1002 summary line hardcoded "later than the latest real SLC" even in
historical mode, where the boundary is the *earliest* real date. The
per-burst detail lines were already correct, so the header contradicted
them and pointed operators at the wrong date. Make the summary mode-aware.
The 1000 message rendered its threshold as `{max_gap_days / 365.25:.0f}
-year`, so any sub-year `max_gap_days` reported a "0-year limit". Render
days below one year, and include the gap in days alongside the fraction.
Found by the precheck scenario suite in the delivery package, which
exercises every numbered code (1000-2001) against real CSLC filenames.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Scoping the checks per burst introduced a call to opera_utils.group_by_burst, which raises a bare ValueError on any filename it can't parse a burst ID from. That turned an unrecognized name into an uncoded crash *before* the prechecks ran, defeating the point of having them report a numeric error_code -- and it broke five existing tests that use synthetic names like "s_20200101.h5", which origin/main handled fine. Group defensively instead: fall back to a single pooled group, restoring the burst-agnostic behavior these checks had before. When only one of the real / compressed lists is unparseable, pool both, so the two sides can't end up keyed differently and silently share no bursts to compare. Also renumber the forward-mode network-depth check from 2002 to 2001, per the agreed scheme (1000-1002 common, 2000-2001 forward-only), and update its tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes several claims in the compressed-SLC docs that didn't match what the code does, and makes the error messages use one consistent vocabulary. Doc corrections (all verified against disp_s1, dolphin and configs/): - The uncaught date-overlap crash is `BaseStack._check_no_date_overlap`, raised when `run_wrapped_phase_sequential` builds its `MiniStackPlanner`. There is no `dolphin.stack.Ministack`, and no forward-only early construction: both modes reach it via `run_displacement` -> `wrapped_phase.run`, after mask/PS/multilook. Neither mode fails at the CLI entry point without the precheck, which is why the precheck exists. - `last_per_ministack` is disp-s1's plan (set by every delivered algorithm_parameters yaml); dolphin's own schema default is `always_first`, reachable only from a hand-built `AlgorithmParameters()`. The `ref == end` identity that 1001/1002 rely on holds only under the former. - `create_ifgs` keeps the whole single-reference group only when `reference_idx == 0`, which never holds operationally. Historical keeps `single_ref_ifgs[:max_bandwidth]`; forward keeps none, since `_create_forward_mode_network` returns a bare `InterferogramNetwork (indexes=...)` (which also discards the forward yaml's `max_bandwidth`). Both are by design, so they're documented as such. - The 0-5 CCSLC bound comes from the triggering logic, not from `max_num_compressed` (the configs leave it at 100). - Document the run_disp.py forward window: `latest_k_per_burst`'s `ref < first_real_date` filter already prevents a real CSLC from landing inside a supplied CCSLC's range, so the previously noted "known gap" only applies under a non-operational compressed_slc_plan. Message wording: real inputs are CSLCs, compressed ones are CCSLCs, applied across codes 1000-2001 and mirrored in docs/error-codes.md. No behavior or error codes change; tests assert on error_code only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects, all in code this branch introduced or touched: - `_create_forward_mode_network` sampled only the first burst for the nearest-N depth check, on the assumption that all bursts share the same acquisition dates. That contradicts the per-burst premise the rest of these checks are built on: a short non-first burst passed validation and still hit the `_make_ifg_pairs` IndexError that error 2001 exists to prevent. Check every burst and name the short ones. - `_assert_no_compressed_slc_conflicts` unpacked `get_dates(f)[:3]` with no length guard. Since compressed-SLC detection matches on the whole path, a real CSLC staged under a directory containing "compressed" is misread as a CCSLC, yields fewer than three dates, and crashed the check with an uncoded IndexError -- the exact failure mode these prechecks exist to replace with coded errors. Skip entries that aren't CCSLC-shaped, while still validating any genuine CCSLC beside them. - `_assert_no_duplicate_dates` runs first in `run()` and called `group_by_burst` unguarded, so it raised on unparseable burst IDs before the new defensive grouping could ever help. Route it through the same helper so the precheck block behaves consistently. Also fix a pre-existing log line that reported the number of bursts as the number of compressed SLCs; the per-burst reference dates now printed beside it made the undercount conspicuous. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rename `docs/compressed-slc-operations.md` to `docs/disp-s1-operations.md`
and follow up on the two things that went stale with it.
`docs/disp-s1-operations.md`:
- The "Output re-referencing" section read as mode-agnostic, but
`_redo_reference` is gated on `product_type == "DISP_S1_FORWARD"` in
`main.py`, with no historical branch. Show the guard in the quoted
snippet and say so.
- Note the two adjacent things that are *not* forward-gated: the
unconditional `second_to_last_date` unpack (whose implicit "at least two
distinct dates" requirement therefore applies in historical too), and
the database-driven `output_options.extra_reference_date`, which runs in
both modes.
`docs/error-codes.md`:
- Three links pointed at the pre-rename filename.
- 1000's quoted message said "input SLCs"; the code emits "input CSLCs".
- 2001's quoted message predated the per-burst fix — it reports
"in every burst, but found only -- {burst_id}: {count}", not
"in the input stack, but only {count} were found".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Lets an operator force a run whose inputs a precheck rejects, without patching the SAS. Default `true`, so nothing changes unless it is set. - `RunConfig.run_input_prechecks` gates 1000/1001/1002/2000 in `disp_s1.main.run`, and logs a warning on the skip path so a disabled run is visible in the log. - 2001 is the one check that runs during `RunConfig.to_workflow` rather than `main.run`. It is gated by withholding `cslc_file_list` from `_create_forward_mode_network`, which that function already documents as "nothing to count, no assertion" -- so the interferogram network built is identical either way, only the depth assertion is skipped. - `_assert_no_duplicate_dates` is deliberately *not* gated: it is uncoded, predates the numbered checks, and two real CSLCs for one date in one burst is an ambiguity no later step can resolve. - Added to `configs/runconfig_historical.yaml`, with the comment block taken verbatim from `RunConfig.print_yaml_schema()`. Not added to `runconfig_static.yaml`: the static-layers workflow never runs prechecks. Tests cover the default, a yaml round-trip, a runconfig with the key absent (must load with prechecks on -- this must never become effectively opt-in), 2001 raised when on and skipped when off, and `main.run` calling exactly the expected checks when on and none when off. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pre-commit.ci was red on three hooks. All of it predates the last commit. mypy: `_group_by_burst` is annotated `dict[str | None, list[Path]]` -- the `None` key is the pooled fallback for unparseable burst IDs -- and mypy follows that annotation into four call sites that `opera_utils`' untyped `group_by_burst` used to hide: - two `sorted()` calls over burst-ID sets, which cannot order `None`; both now pass `key=str`. - two `str.join` calls over `list[Path]` and `list[str | None]`; both now stringify. The 2000 message reuses the `burst_id or "input CSLC list"` wording already used by the duplicate-date check for a pooled group. No behavior change for real OPERA inputs, whose burst IDs always parse. Also applied the hooks' own fixes: black (`--preview` with `string_processing`) rejoined two implicit string concatenations, and trailing-whitespace trimmed one line in `docs/disp-s1-operations.md`. `autofix_prs` is false for this repo, so these have to be pushed by hand. Suite: 75 passed, 2 skipped. Delivery scenario suite: 45/45. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validates compressed-SLC (CCSLC) inputs up front, and fixes an off-by-one in the
forward-mode network guard that let a too-shallow stack through into an
IndexErrordeep inside dolphin.Two of these were found by running the delivery package: one is a genuine crash
on a valid-looking config, the other cost ~90 s of processing before failing with
an unactionable pydantic traceback.
The bugs
1. Forward-mode stack-depth guard counted compressed SLCs (real crash).
_create_forward_mode_networkbuilds a manual-index interferogram networkreaching back
nearest_n + 1positions, and asserted the stack had at least thatmany files. But
dolphin.workflows.wrapped_phase.runcollects that network'sinputs by globbing only the real-date phase-linked outputs (
"2*.tif") —compressed-SLC outputs are globbed separately (
"compressed*tif") and neverenter the list. So a CCSLC adds no depth, and a stack of e.g. 2 CCSLC + 3 real
passed a nearest-3 guard needing 4, then died with:
Now counts real SLCs only.
The same guard also sampled a single burst as a template for the whole stack
(
burst_id = next(iter(burst_to_file_list))), on the assumption that every burstshares the same acquisition dates. A short non-first burst therefore reached the
same
IndexError. Every burst is now checked, and the message names the shallowones:
... in every burst, but found only -- t027_056726_iw1: 3.2. CCSLC/real-CSLC date conflicts weren't checked at all.
Two malformed shapes were accepted by disp-s1 and only rejected mid-workflow by
dolphin's ministack construction, as a pydantic
ValidationErrorwith noindication of which input was at fault. In historical mode that happens after
mask extraction and PS selection have already run -- the
MiniStackPlannerconstruction that raises is the first statement in
run_wrapped_phase_sequential,so the cost is paid before any phase linking starts.
3. Forward mode accepted a burst with real CSLCs but no CCSLC of its own
(2000). Each burst is processed independently, so that burst gets an empty
compressed-SLC reader downstream and fails in
disp_s1._ps.run_combine.Mode-awareness of 1002
The boundary a CCSLC must predate depends on how many products the mode emits:
DISP_S1_FORWARDThis relies on
compressed_slc_plan="last_per_ministack", under which a CCSLC'sreference date is by construction its end date. That is DISP-S1's setting, not
dolphin's default: every delivered
configs/algorithm_parameters_*.yamlsets it,while dolphin's own
PhaseLinkingOptionsdefaults toalways_first(under whichthe identity does not hold). That's stated explicitly at the check.
Error codes
1000–1002apply in every mode;2000–2001are forward-only.Important
This renumbers the network-depth check from
2002to2001, so the codesform two contiguous blocks.
2002shipped in v0.5.14; that release is beingre-cut with these fixes, so nothing should be pinned to the old number — but
it is a deliberate breaking change to an already-published code, and worth a
look if anything downstream maps it.
Other changes
_assert_no_large_temporal_gapsis now scoped per burst and reports everyoffending gap at once rather than only the first. Bursts are processed
independently, so one burst's coverage shouldn't paper over a genuine hole in
another's. It also takes a plain file list; the
Mappingoverload had onecaller and no test coverage.
opera_utils.group_by_burstraises a bareValueErroron filenames it can't parse a burst ID from, which would replacea coded precheck failure with an uncoded crash before any check ran. It now
falls back to a single pooled group, preserving the pre-existing behavior for
such names. (Real OPERA CSLC/CCSLC names always parse; this is about not
turning an unexpected name into a confusing failure.)
_assert_forward_mode_compressed's docstring described a "connected to thestack" reference-date-overlap check that was never implemented in the body.
Replaced with a description of what the function actually does.
so a forward run's log shows what the next run will consume.
run_input_prechecks(defaulttrue) turns everynumbered check on or off from the runconfig yaml. It is an operational escape
hatch for forcing a run whose inputs a precheck rejects; off, a malformed input
list fails inside dolphin or not at all. A runconfig predating the field loads
with the prechecks on. 2001 is gated by withholding
cslc_file_listfrom_create_forward_mode_network, so the network built is identical either way.The uncoded duplicate-date check is deliberately not gated.
docs/error-codes.md(every code, its message, cause and fix) anddocs/disp-s1-operations.md(CCSLC operational logic, modedifferences, and the filename conventions for both the self-generated
compressed_*form and the officialOPERA_L2_COMPRESSED-CSLC-S1_*form).Testing
burst-grouping fallback (including the mixed-parseability case, where pooling
only one side would silently skip the check).
run_input_prechecks: the default, a yaml round-trip, arunconfig with the key absent (must load with the prechecks on), 2001 raised
when on and skipped when off, and
main.runcalling exactly the expectedchecks when on and none when off.
negative control, including that the same straddling
stack fails 1002 in historical and passes in forward, and that every check
fires when just one of the two bursts violates it (with the message naming
that burst alone) -- 45/45.
delivery_data_official, and their products validate against the goldendataset. (That run predates
run_input_prechecks; with the flag at itsdefault the code path through both modes is unchanged.)