Conversation
Add a canonical sarpyx.snapflow.snap2stamps module with upstream-equivalent TOPSAR and Stripmap processing branches. Keep snap2stamps_pipelines as a compatibility shim, update package exports, refresh the notebook and guide imports, and expand tests around branch selection and dispatch.
Correct pair preparation defaults for TOPSAR coregistration, make named TOPSAR pipeline dispatch honor the requested variant, and separate single-input versus pair workflow execution.\n\nAdd regression coverage for the restored contracts and workflow input metadata.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b47a423260
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pipeline_name = select_topsar_export_pipeline( | ||
| master_count=master_count, | ||
| external_dem_file=external_dem_file, | ||
| ) |
There was a problem hiding this comment.
run_processing_pipeline is documented as a named dispatcher, but run_topsar_export immediately recomputes pipeline_name from master_count/external_dem_file instead of using the variant the caller requested. This means a call like topsar_export_mergeiw_subset_ext_dem can silently execute the non-_ext_dem branch (or vice versa) when arguments differ, producing a different DEM-processing path than requested and making results non-reproducible.
Useful? React with 👍 / 👎.
| if pipeline_name.startswith("stripmap_dem_assisted_coregistration"): | ||
| if pair is None: | ||
| raise ValueError(f"{pipeline_name} requires pair=PairProducts(...)") | ||
| return run_stripmap_coreg(pair=pair, **common, **kwargs) |
There was a problem hiding this comment.
The stripmap_dem_assisted_coregistration_ext_dem named branch is dispatched without checking that external_dem_file is provided, so callers can request the ext-DEM pipeline and still run with default DEM settings. This silently executes a different processing branch than the requested pipeline variant and can alter coregistration outputs.
Useful? React with 👍 / 👎.
| if pipeline_name.startswith("stripmap_interferogram_topophase"): | ||
| if source_product is None: | ||
| raise ValueError(f"{pipeline_name} requires source_product") | ||
| return run_stripmap_ifg(coreg_product=source_product, **common, **kwargs) |
There was a problem hiding this comment.
The stripmap_interferogram_topophase_ext_dem branch is also dispatched without enforcing external_dem_file, and run_stripmap_ifg then falls back to SRTM 1Sec HGT. In this case the _ext_dem pipeline name does not guarantee external-DEM processing, so users can unknowingly generate topophase results from the wrong DEM source.
Useful? React with 👍 / 👎.
Update the static docs generator and regenerate the inventory-driven site so sarpyx.snapflow.snap2stamps is documented as the canonical API path, with snap2stamps_pipelines kept as a deprecated compatibility alias.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ee3391060
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| snap_userdir=snap_userdir, | ||
| ) | ||
| selected_pols = [polarisation] if polarisation else None | ||
| if select_topsar_split_pipeline(len(products)) == "topsar_assemble_split_applyorbit": |
There was a problem hiding this comment.
run_topsar_split_apply_orbit recomputes the branch from len(source_products) and conditionally runs slice_assembly, so an explicit call to run_processing_pipeline("topsar_split_applyorbit", ...) can silently execute the assemble variant (and vice versa). This breaks the contract of named pipeline dispatch and can change preprocessing outputs depending on runtime list length instead of the requested pipeline name.
Useful? React with 👍 / 👎.
| if pipeline_name.startswith("stripmap_dem_assisted_coregistration"): | ||
| if pair is None: | ||
| raise ValueError(f"{pipeline_name} requires pair=PairProducts(...)") | ||
| return run_stripmap_coreg(pair=pair, **common, **kwargs) |
There was a problem hiding this comment.
The dispatcher sends both stripmap_dem_assisted_coregistration variants through the same call and forwards **kwargs unchanged, so if a caller selects the non-_ext_dem pipeline but passes external_dem_file (for example via shared config), run_stripmap_coreg still runs the External DEM path. That means the executed coregistration branch can differ from the explicitly requested pipeline name.
Useful? React with 👍 / 👎.
Pin Docker Buildx to a stable version to avoid transient default-image failures and restrict Docker and publish workflows to main-branch execution paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a72c0dffc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if coreg_products is None or ifg_products is None: | ||
| raise ValueError(f"{pipeline_name} requires coreg_products and ifg_products") |
There was a problem hiding this comment.
topsar_export is registered as input_kind="pair" and pipeline_requires_pair("topsar_export") therefore reports that callers should provide a pair, but this dispatcher branch ignores pair and raises unless coreg_products/ifg_products are passed. In practice, run_processing_pipeline("topsar_export", pair=PairProducts(...), ...) fails despite the pipeline metadata advertising a pair-based interface, which makes the helper contract inconsistent and breaks callers that validate inputs via the registry.
Useful? React with 👍 / 👎.
1 participant
Summary
notebook imports to the canonical path
the follow-up API regressions around pair prep, named branch dispatch, and workflow
execution contracts
What Changed
helpers
Regression Fixes
coregistration receives valid burst-level inputs
silently reselecting from runtime parameters
callable through the correct entrypoint
Verification
tests/test_snap2stamps_pipelines.py
test_snap2stamps_pipelines.py -q
Test result:
Notes
CLI/config runner or plotting scripts
avoid breaking existing imports