Skip to content

Add polarization-specific validDataMask datasets to InSAR product - #379

Open
xhuang-jpl wants to merge 52 commits into
isce-framework:developfrom
xhuang-jpl:fix_insar_mask
Open

xhuang-jpl wants to merge 52 commits into
isce-framework:developfrom
xhuang-jpl:fix_insar_mask

Conversation

@xhuang-jpl

Copy link
Copy Markdown
Contributor

Summary

Add polarization-specific validMask datasets to InSAR products for tracking reference and secondary image validity per polarization.

Changes

  • New Dataset: validMask (uint8) with bit encoding - bit 1: reference validity, bit 0: secondary validity
  • Geocoding: Added get_valid_mask_input_output() and updated CPU/GPU workflows to geocode validMask
  • Writers: Modified GUNW, GOFF, ROFF, and L1 writers to create validMask in pixelOffsets, unwrappedInterferogram, and wrappedInterferogram groups

Xiaodong Huang added 30 commits September 19, 2023 20:40
@s-sasaki-earthsea-wizard

Copy link
Copy Markdown

Thanks for integrating the row-wise generate_insar_mask vectorization from #359 into this PR — and the sliding-window _RSLCInputDataExceptionMask reader is a genuine improvement over #359's full-array loads, as the numbers below show.

I ran this PR's head (0c2ff0af5) against its merge-base (0d1600d8b) on a real NISAR L-band frame (ascending 139/019 pair, frequency A / HH, RIFG interferogram grid 6840 x 10581 = 72.4 Mpx), as a standalone prepare_insar_hdf5 run against a pre-seeded geo2rdr scratch — the same harness as the #358/#359 comparison posted here. Only the 7 files this PR changes differ between the runs.

variant prepare_insar_hdf5 (journal) vs scalar peak RSS
merge-base (scalar loop) 418.0 s 1x 12.5 GiB
this PR 84.8 s 4.9x 6.6 GiB

No regressions on this dataset. All 558 common datasets across the GUNW skeleton and the RIFG/RUNW scratch skeletons are byte-identical to the merge-base run except the usual run-varying metadata (processingDateTime, runConfigurationContents); that includes every existing mask layer. The only structural additions are the 7 new validMask datasets, and the radar-grid ones check out per pixel: HH validMask equals the sub-swath validity bits of the co-located mask (2*(ref digit > 0) + (sec digit > 0)) with zero mismatches on all four RIFG/RUNW layers (RIFG interferogram: 69,341,858 px with either image valid, 68,664,482 with both; pixelOffsets: 368,611 / 366,145). The GUNW validMask planes are creation-state skeletons at the prepare stage, so their geocoding is not covered by this check. Peak RSS lands below both the scalar baseline and #359 (10.1 GiB on the same harness), thanks to the windowed exception-mask reads. Full logs, provenance, and the comparison scripts: artifacts/pr379-ab-20260913/.

The performance and the tested-configuration results look good. Reading the integrated code turned up two validMask correctness notes in input configurations beyond the tested one, plus two minor cleanups — details and suggested fixes are folded below.

Two validMask correctness notes (multi-frequency products; non-HH polarizations with a uint8 or missing inputDataExceptionMask) — worth addressing before merge; neither manifests on the frame above
  1. The per-pol validMask update for pixelOffsets runs outside the frequency loop. In InSAR_L1_writer.add_pixel_offsets_to_swaths_group, the for pol in pol_list: block that writes pixelOffsets/{pol}/validMask (InSAR_L1_writer.py#L570-L583) sits at method level, while pol_valid_mask, pol_list, and offset_group_name are loop variables of the for freq, pol_list, _ in get_cfg_freq_pols(...) loop starting at L371. For a multi-frequency product only the last frequency's validMask is written; frequency A's stays at its creation bytes (all zeros = "both invalid", with _FillValue = 255). The interferogram counterpart in add_interferogram_to_swaths_group is correctly inside the loop. Note that indenting the update block alone is not quite enough: _add_datasets_to_pixel_offset_group(), which creates the per-pol validMask datasets, also runs after the loop (L568), so each frequency's dataset creation and validMask write need to happen inside the same frequency scope, in that order.

  2. The fallback for RSLCs without per-pol validity bits populates only the HH position. For RSLCs with a pre-RSLC missing data mask #370 uint8 inputDataExceptionMask — and equally when the dataset is absent — the fallback sets bit 8 (reference) / bit 0 (secondary) from sub-swath validity (utils.py#L893-L894, L914-L915). Those are the _PolBit.HH positions, so extract_pol_valid_mask returns all-invalid for every other polarization of such granules (e.g. HV of a DHDH pair). A minimal repro (in the artifact bundle linked above) confirms it at runtime: on a fully-valid grid with zero offsets, mask is 11 everywhere while the extracted validMask is HH=3 and HV/VH/VV=0, for both a uint8 and an absent exception mask. If the intent is "no per-pol information, fall back to sub-swath validity for all pols", broadcasting the fallback to all eight bit positions (* 0xFF00 / * 0x00FF) would preserve that.

Minor cleanups (fine as a follow-up)
  1. _subswath_numbers is defined twice in utils.py. The first definition (L497, the verbatim Add regression tests for generate_insar_mask #359 version) is shadowed by the second at L703 and can be deleted.

  2. pixel_offsets_ds_params in InSAR_L1_writer._add_datasets_to_pixel_offset_group lists slantRangeOffset twice (L328, L335) — harmless through require_dataset, but presumably unintended.

On #359: since this PR carries the implementation, I'll downshift #359 to its regression tests only (the offer from the #358/#359 threads). The suite passes against this PR's head as-is — 11/11, including the adversarial rounding and bit-packing cases — so it can serve as the regression gate for the uint32 mask semantics of the integrated implementation (it also sanity-checks the per-pol validity word against the mask's sub-swath digits; the finer per-pol bit semantics are this PR's own and are best covered by its tests).

@hfattahi hfattahi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very minor comments before we go to the details of this PR.

Also one note that we need to discuss in a PR review:

what is the behavior when the workflow runs on older/existing RSLCs with 8 bits inputDataExceptionMask. Ideally we should probably populate the InSAR validDataMask inside polarization group with the same subswathValidDataMask in "mask" dataset.

)
self._create_2d_dataset(
pixeloffsets_pol_group,
"validMask",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we already have subswath Valid Data Mask suggest

Suggested change
"validMask",
"validDataMask",

goff_shape,
np.uint8,
(f"Valid mask for the {pol} layers: "
"bit 1 = reference (1=valid, 0=invalid), bit 0 = secondary (1=valid, 0=invalid)"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expand and make the description more informative. Maybe something like this:

Suggested change
"bit 1 = reference (1=valid, 0=invalid), bit 0 = secondary (1=valid, 0=invalid)"),
"bit 1 = reference (1=valid, 0=invalid), bit 0 = secondary (1=valid, 0=invalid). Valid represents fully focused data and invalid represents partially focused or missing data"),

@bhawkins please take a look if that extra description is consistent with RSLC.

(
"validMask",
np.uint8,
f"Valid mask for the {pol} layers: bit 1 = reference (1=valid, 0=invalid), bit 0 = secondary (1=valid, 0=invalid)",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar comment as above about more clear description.

@bhawkins bhawkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's what we came up with looking though this today. However, we ran out of time, and I may need to revise some of these comments once I have a better understanding of what's going on here.

Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py
@s-sasaki-earthsea-wizard

s-sasaki-earthsea-wizard commented Sep 17, 2026

Copy link
Copy Markdown

@hfattahi
On the older-RSLC / 8-bit inputDataExceptionMask question: that case is note 2 of my earlier comment. The fallback currently sets only the HH bit positions (utils.py#L779-L780, #L800-L801), so extract_pol_valid_mask returns all-invalid for every non-HH polarization of such a granule — confirmed at runtime on a fully-valid grid (mask = 11 everywhere, extracted validMask HH=3 and HV/VH/VV=0), for both a uint8 and an absent exception mask. Broadcasting the fallback to all eight bit positions (* 0xFF00 / * 0x00FF) would give exactly the "same sub-swath validity for all polarizations" behavior you describe; the repro script is in the artifact bundle linked from that comment.

For bookkeeping: c7a7e86fe covers note 3 of that comment; notes 1 and 4 still apply to the current head.

@xhuang-jpl

Copy link
Copy Markdown
Contributor Author

Thanks @s-sasaki-earthsea-wizard for finding those major bugs, which have been fixed. @hfattahi if it is old RSLC uint8, it will fallback to the validSubswathMask, but its values are only 1 or 0, not the subswath mask number.

Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/InSAR_L1_writer.py
Comment thread python/packages/nisar/workflows/geocode_insar.py Outdated
Comment thread python/packages/nisar/workflows/geocode_insar.py
Comment thread python/packages/nisar/workflows/geocode_insar.py Outdated
Comment thread python/packages/nisar/workflows/geocode_insar.py Outdated
@xhuang-jpl xhuang-jpl changed the title Add polarization-specific validMask datasets to InSAR product Add polarization-specific validDataMask datasets to InSAR product Sep 21, 2026
Comment thread python/packages/nisar/products/insar/utils.py Outdated
Comment thread python/packages/nisar/products/insar/utils.py

@bhawkins bhawkins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for all your hard work on this patch.

@hfattahi

Copy link
Copy Markdown
Contributor

@xhuang-jpl Preferably let's merge Jungkyo's PR first #381 as there seems to be minor conflict that you will need to resolve.

@hfattahi hfattahi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @xhuang-jpl for this great addition.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants