Add regression tests for generate_insar_mask - #359
Add regression tests for generate_insar_mask#359s-sasaki-earthsea-wizard wants to merge 1 commit into
Conversation
|
Following up on the cross-check I posted on #358 with the real-data leg — posting it here since this PR is where the I re-ran the three-way comparison on a real NISAR L-band frame (ascending 139/019 Boso pair, freq A: RIFG interferogram grid 6840 x 10581 = 72.4 Mpx, RUNW 2565 x 4069, plus the pixel-offset masks), as a standalone
Bitwise equivalence holds on real data for both implementations. All 558 datasets across the GUNW skeleton and the RIFG/RUNW scratch skeletons were compared byte-for-byte against the scalar run: for both #358 and #359 the only differences are the run-varying metadata ( Where the two differ at production scale is memory, more than speed. The full-grid formulation holds several ~579 MB temporaries concurrently on the 72.4 Mpx RIFG grid, putting the step's peak at 17.9 GiB — +5.5 GiB over the scalar baseline. The row-wise formulation peaks at 10.1 GiB, below the scalar baseline (the scalar loop's 72.4M-element Python list of ints, ~2 GiB, is gone). The ~12.4 GiB floor common to all three is the workflow's own working set (RSLC-grid exception masks etc.). The wall-clock difference between the two vectorizations is real but secondary at step scale (~1.2x). Since NISAR PGE workers run under memory budgets, this seemed worth having on the record before any implementation decision, so I am not holding it back. Full logs, provenance (variant SHAs/md5s, seed, environment), and the comparison script are public in my benchmark repo: The offer from the #358 thread stands unchanged: I am happy with either implementation landing — if the maintainers prefer #358's for the prepare part, I can rebase this PR down to its regression tests. |
|
Part of codes in this PR to improve the |
Cover the semantics the vectorized implementation must preserve: - _subswath_numbers against the scalar SubSwaths.get_sample_sub_swath API as oracle: out-of-bounds -> 0, first-match-wins ordering, the empty valid-samples-array short-circuit, and the no-sub-swath-information -> 1 path; - generate_insar_mask against a per-pixel scalar reference (_compute_subswath_mask_id plus Python-int bit packing) on synthetic fixtures with adversarial offsets (exact k + 0.5 half-integers, large out-of-swath pushes), differing reference/secondary dimensions, empty and absent sub-swath layouts, and the missing inputDataExceptionMask dataset path; - exact uint32 packing of exception-mask bytes with the MSB set, which the previous uint8-scalar << 16 / << 8 shifts silently drop under NumPy >= 2 scalar promotion (isce-framework#335); - the rounding asymmetry between the sub-swath lookup (int(x + 0.5), truncation toward zero) and the exception-mask lookup (round-half-even), including a negative secondary index where truncation toward zero and floor diverge. The seven behavior tests pass unchanged against the pre-vectorization scalar implementation under NumPy 1.26, confirming they encode the existing semantics rather than the new implementation's.
dbb4e8d to
dab52a7
Compare
|
@xhuang-jpl Thanks for the heads-up — glad the row-wise implementation is useful in #379. Following the standing offer from this thread: since #379 now carries the |
Summary
Downshifts this PR to regression tests only, per the offer in the #358/#359 threads: the
generate_insar_maskvectorization originally proposed here has been integrated into #379 (thanks @xhuang-jpl), so the implementation commit is dropped and what remains is the test suite that pins the mask semantics for any implementation.What the tests pin
SubSwaths.get_sample_sub_swathequivalence: out-of-bounds -> 0, first-match-wins sub-swath ordering, an empty valid-samples array claims every in-bounds sample, no-sub-swath-info assigns 1 in bounds;int(x + 0.5), exception-mask lookup rounds half to even), exercised at exactk + 0.5landings where they diverge;(ref << 16) | (sec << 8)exception-byte packing, with MSB-set bytes chosen so that fixed-width uint8 scalar promotion (silently dropped bits, NEP 50 semantics included) would be caught;_compute_subswath_mask_idhelper with Python-int bit packing (so the oracle cannot inherit a fixed-width overflow). The helper is currently retained by Add polarization-specificvalidMaskdatasets to InSAR product #379; these tests rely on it staying importable — if it is ever removed, the oracle should be inlined here instead.Scope and compatibility
The suite is a regression gate for the uint32
masksemantics (including the legacy uint8 / absentinputDataExceptionMaskpath, which is what the fixtures feed). It runs against the current scalar implementation ondevelop(7 passed, 4 skipped — the helper-level_subswath_numberstests skip when no vectorized helper exists and activate automatically once one lands) and against #379's head as-is (11 passed). A smallunpack_maskshim accepts both the single-array return and #379's two-element(mask, pol_valid_mask)tuple; when the tuple is present, the per-pol validity word is sanity-checked against the mask's sub-swath digits (validity bits set exactly where the corresponding sub-swath digit is nonzero) before the mask is compared. The finer per-pol bit semantics and the uint16 exception-mask path introduced by #370/#379 are intentionally out of scope here and are best covered by #379's own tests. Test runs above used numpy 1.26.4.Follow-up once #379 lands
The transition affordances should then be removed: make the
_subswath_numbersimport unconditional (dropping theskipif) and unpack the tuple directly (dropping the single-array branch of the shim), so signature drift fails loudly instead of being tolerated.