Add whirlwind unwrapping algorithm - #367
Conversation
|
Rebasing with develop should solve the unit test failures. |
|
initial things-
|
scottstanie
left a comment
scottstanie
left a comment
There was a problem hiding this comment.
Here's some suggestions, and xhuang-jpl/isce3@add_whirlwind...scottstanie:isce3:whirlwind-followups is a branch off of this which has a few more. i suggest cutting some of these options, since i believe it's very unlikely to be useful (especially the alternatve conncomp_algorithm. the snaphu one is good, the other maybe we should cut).
| # conncomp_min_coherence is None. 0 labels essentially every reliably unwrapped | ||
| # pixel; raise to cut more low-coherence interior edges. Only used when | ||
| # conncomp_algorithm="snaphu". | ||
| conncomp_reliability: 0.0 |
There was a problem hiding this comment.
| conncomp_reliability: 0.0 | |
| conncomp_reliability: 0.5 |
| def set_whirlwind_attributes(cfg: dict): | ||
| """ | ||
| Return dictionary with whirlwind parameters from user-defined config | ||
|
|
||
| Parameters | ||
| ---------- | ||
| cfg: dict | ||
| Dictionary containing user-defined whirlwind parameters | ||
|
|
||
| Returns | ||
| ------- | ||
| ww_kwargs: dict | ||
| Dictionary of whirlwind parameters for whirlwind.unwrap() | ||
| """ | ||
| ww_kwargs = {} | ||
|
|
||
| # Add parameters if specified in config | ||
| if cfg.get('bridge') is not None: | ||
| ww_kwargs['bridge'] = cfg['bridge'] | ||
| if cfg.get('downsample') is not None: | ||
| ww_kwargs['downsample'] = cfg['downsample'] | ||
| if cfg.get('interpolate') is not None: | ||
| ww_kwargs['interpolate'] = cfg['interpolate'] | ||
| if cfg.get('interp_cutoff') is not None: | ||
| ww_kwargs['interp_cutoff'] = cfg['interp_cutoff'] | ||
| if cfg.get('interp_num_neighbors') is not None: | ||
| ww_kwargs['interp_num_neighbors'] = cfg['interp_num_neighbors'] | ||
| if cfg.get('interp_max_radius') is not None: | ||
| ww_kwargs['interp_max_radius'] = cfg['interp_max_radius'] | ||
| if cfg.get('interp_min_radius') is not None: | ||
| ww_kwargs['interp_min_radius'] = cfg['interp_min_radius'] | ||
| if cfg.get('interp_alpha') is not None: | ||
| ww_kwargs['interp_alpha'] = cfg['interp_alpha'] | ||
| if cfg.get('conncomp_algorithm') is not None: | ||
| ww_kwargs['conncomp_algorithm'] = cfg['conncomp_algorithm'] | ||
| if cfg.get('conncomp_min_coherence') is not None: | ||
| ww_kwargs['conncomp_min_coherence'] = cfg['conncomp_min_coherence'] | ||
| if cfg.get('conncomp_reliability') is not None: | ||
| ww_kwargs['conncomp_reliability'] = cfg['conncomp_reliability'] | ||
| if cfg.get('cost_threshold') is not None: | ||
| ww_kwargs['cost_threshold'] = cfg['cost_threshold'] | ||
| if cfg.get('conncomp_cycle_prob') is not None: | ||
| ww_kwargs['conncomp_cycle_prob'] = cfg['conncomp_cycle_prob'] | ||
| if cfg.get('conncomp_sigma') is not None: | ||
| ww_kwargs['conncomp_sigma'] = cfg['conncomp_sigma'] | ||
| if cfg.get('min_size_px') is not None: | ||
| ww_kwargs['min_size_px'] = cfg['min_size_px'] | ||
| if cfg.get('max_ncomps') is not None: | ||
| ww_kwargs['max_ncomps'] = cfg['max_ncomps'] | ||
| if cfg.get('goldstein_alpha') is not None: | ||
| ww_kwargs['goldstein_alpha'] = cfg['goldstein_alpha'] | ||
| if cfg.get('goldstein_psize') is not None: | ||
| ww_kwargs['goldstein_psize'] = cfg['goldstein_psize'] | ||
|
|
||
| return ww_kwargs | ||
|
|
||
|
|
There was a problem hiding this comment.
i think this whole thing could be condensed to list the keys in a module level tuple and then make it
| def set_whirlwind_attributes(cfg: dict): | |
| """ | |
| Return dictionary with whirlwind parameters from user-defined config | |
| Parameters | |
| ---------- | |
| cfg: dict | |
| Dictionary containing user-defined whirlwind parameters | |
| Returns | |
| ------- | |
| ww_kwargs: dict | |
| Dictionary of whirlwind parameters for whirlwind.unwrap() | |
| """ | |
| ww_kwargs = {} | |
| # Add parameters if specified in config | |
| if cfg.get('bridge') is not None: | |
| ww_kwargs['bridge'] = cfg['bridge'] | |
| if cfg.get('downsample') is not None: | |
| ww_kwargs['downsample'] = cfg['downsample'] | |
| if cfg.get('interpolate') is not None: | |
| ww_kwargs['interpolate'] = cfg['interpolate'] | |
| if cfg.get('interp_cutoff') is not None: | |
| ww_kwargs['interp_cutoff'] = cfg['interp_cutoff'] | |
| if cfg.get('interp_num_neighbors') is not None: | |
| ww_kwargs['interp_num_neighbors'] = cfg['interp_num_neighbors'] | |
| if cfg.get('interp_max_radius') is not None: | |
| ww_kwargs['interp_max_radius'] = cfg['interp_max_radius'] | |
| if cfg.get('interp_min_radius') is not None: | |
| ww_kwargs['interp_min_radius'] = cfg['interp_min_radius'] | |
| if cfg.get('interp_alpha') is not None: | |
| ww_kwargs['interp_alpha'] = cfg['interp_alpha'] | |
| if cfg.get('conncomp_algorithm') is not None: | |
| ww_kwargs['conncomp_algorithm'] = cfg['conncomp_algorithm'] | |
| if cfg.get('conncomp_min_coherence') is not None: | |
| ww_kwargs['conncomp_min_coherence'] = cfg['conncomp_min_coherence'] | |
| if cfg.get('conncomp_reliability') is not None: | |
| ww_kwargs['conncomp_reliability'] = cfg['conncomp_reliability'] | |
| if cfg.get('cost_threshold') is not None: | |
| ww_kwargs['cost_threshold'] = cfg['cost_threshold'] | |
| if cfg.get('conncomp_cycle_prob') is not None: | |
| ww_kwargs['conncomp_cycle_prob'] = cfg['conncomp_cycle_prob'] | |
| if cfg.get('conncomp_sigma') is not None: | |
| ww_kwargs['conncomp_sigma'] = cfg['conncomp_sigma'] | |
| if cfg.get('min_size_px') is not None: | |
| ww_kwargs['min_size_px'] = cfg['min_size_px'] | |
| if cfg.get('max_ncomps') is not None: | |
| ww_kwargs['max_ncomps'] = cfg['max_ncomps'] | |
| if cfg.get('goldstein_alpha') is not None: | |
| ww_kwargs['goldstein_alpha'] = cfg['goldstein_alpha'] | |
| if cfg.get('goldstein_psize') is not None: | |
| ww_kwargs['goldstein_psize'] = cfg['goldstein_psize'] | |
| return ww_kwargs | |
| def set_whirlwind_attributes(cfg: dict): | |
| """ | |
| Return dictionary with whirlwind parameters from user-defined config | |
| Parameters | |
| ---------- | |
| cfg: dict | |
| Dictionary containing user-defined whirlwind parameters | |
| Returns | |
| ------- | |
| ww_kwargs: dict | |
| Dictionary of whirlwind parameters for whirlwind.unwrap() | |
| """ | |
| return {key: cfg[key] for key in WHIRLWIND_OPTIONS if cfg[key] is not None} | |
adding something like this to the top
+# `whirlwind.unwrap`. `nlooks` and `mask` are excluded because they need
+# conversion (looks estimation, raster read) and are set by the caller.
+WHIRLWIND_OPTIONS = (
+ 'bridge', 'downsample', 'interpolate', 'interp_cutoff',
+ 'interp_num_neighbors', 'interp_max_radius', 'interp_min_radius',
+ 'interp_alpha', 'conncomp_min_coherence','conncomp_reliability',
+ 'goldstein_psize', 'goldstein_alpha', 'min_size_px', 'max_ncomps',
+)| # Get whirlwind parameters using helper function | ||
| ww_kwargs = set_whirlwind_attributes(whirlwind_cfg) | ||
| ww_kwargs['nlooks'] = float(nlooks) | ||
| ww_kwargs['mask'] = mask_array if mask_array is not None else None |
There was a problem hiding this comment.
since above we always make a mask_array,
| ww_kwargs['mask'] = mask_array if mask_array is not None else None | |
| ww_kwargs['mask'] = mask_array |
|
also, didn't notice at first, but the bridging needs to be deduplicated (i would pick the whirlwind one like this xhuang-jpl@bf6aeae ), or else it runs twice |
…o add_whirlwind
@Tyler-g-hudson can you help with this? Thanks |
|
Thank you @hfattahi and @scottstanie , I have made the changes based on you comments, However, for adding the whirlwind as a dependency, I might need help from @Tyler-g-hudson since we also need to update the spec.txt in addition to the environment.yml |
hfattahi
left a comment
hfattahi
left a comment
There was a problem hiding this comment.
I have one more comments for cleaner code and please add whirlwind to CI so that unit test pass.
| info_channel.log("Unwrapping with whirlwind") | ||
|
|
||
| # Get whirlwind dictionary with user params | ||
| whirlwind_cfg = unwrap_args["whirlwind"] | ||
|
|
||
| # Get input array to run unwrapping with whirlwind-insar | ||
| igram_array = open_raster(igram_path) | ||
| coh_array = open_raster(corr_path) | ||
|
|
||
| mask_array = open_raster( | ||
| whirlwind_cfg['mask']) if whirlwind_cfg['mask'] is not None else None | ||
|
|
||
| # Combine the snaphu and preprocessing mask | ||
| if mask is not None: | ||
| mask_array = ~mask if mask_array is None else mask_array & ~mask | ||
|
|
||
| # Get effective number of looks | ||
| if whirlwind_cfg['nlooks'] is not None: | ||
| nlooks = whirlwind_cfg['nlooks'] | ||
| else: | ||
| rg_spacing = src_h5[f"{src_freq_group_path}/interferogram/slantRangeSpacing"][()] | ||
| az_spacing = src_h5[f"{src_freq_group_path}/interferogram/sceneCenterAlongTrackSpacing"][()] | ||
| rg_bw = src_h5[f"{src_freq_bandwidth_group_path}/rangeBandwidth"][()] | ||
| az_bw = src_h5[f"{src_freq_bandwidth_group_path}/azimuthBandwidth"][()] | ||
| nlooks = get_effective_looks(ref_slc, ref_orbit, rg_spacing, | ||
| az_spacing, rg_bw, az_bw, freq=freq) |
There was a problem hiding this comment.
Looks like this part is duplicated code compared with existing code for Snaphu option. Would you please try to combine the two. Maybe something like this:
if algorithin in ["anphu", "whirlwind"]:
code to prep the mask
if alg == snaphu:
run snaphu
else:
run whirlwind
|
To pass CI, please add whirlwind to this environment file https://github.com/isce-framework/isce3/blob/develop/environment.yml Please also add whirlwind to the docker env here: https://github.com/isce-framework/isce3/blob/develop/tools/imagesets/oracle8conda/runtime/environment.yml When the PR is merged @jshimada47 may want to update this spec file: https://github.com/isce-framework/isce3/blob/develop/tools/imagesets/oracle8conda/runtime/spec-file.txt |
|
@scottstanie there is no whirlwind-insar package in the conda-forge channel for the osx platform? cc: @hfattahi |
|
do you mean osx, but not arm? |
It is the osx-arm64 not the osx-64. The CI failure is because we use the macos-latest runner, and it point at the osx-arm64. @scottstanie |
The feedstock has only ever built linux-64, osx-64 and win-64, so there is no Apple Silicon package. That is the mac platform that matters now: `macos-latest` GitHub runners are arm64, so any downstream CI that puts `whirlwind-insar` in a conda environment fails to solve on macOS (isce-framework/isce3#367), while the osx-64 package we do ship is for the Intel macs that are on the way out. Nothing blocks it: python-abi3 is noarch, cargo-bundle-licenses has an osx-arm64 build, and upstream already builds and tests arm64 macOS wheels for every release. Built natively rather than cross-compiled, matching how other rust/maturin feedstocks do it.
https://anaconda.org/channels/conda-forge/packages/whirlwind-insar/files version 0.10.0 has it now |
…o add_whirlwind
|
note that 0.10 also added these 2 options which were helping in the fixed PRF cases to have fewer jumps in between the disconnected subswaths: https://github.com/isce-framework/dolphin/pull/742/changes#diff-b4aa92505a629feacbbd6c3d57b0944330f64c51bc19a7274dc7af58aec8457cR45-R46 |
hfattahi
left a comment
hfattahi
left a comment
There was a problem hiding this comment.
Thank you @xhuang-jpl and @scottstanie. LGTM!
4 participants
This PR adds support for the whirlwind phase unwrapping algorithm to the InSAR workflow.