support HyP3 INSAR_GAMMA Sentinel-1D#1496
Open
Alex-Lewandowski wants to merge 2 commits into
Open
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideExtends HyP3 INSAR_GAMMA product filename parsing and tests to support Sentinel-1D (S1D*) products by broadening the Sentinel-1 mission code pattern and adding a regression test. Flow diagram for updated INSAR_GAMMA filename parsingflowchart TD
A[Input filename] --> B{matches INSAR_ISCE_SINGLE?}
B -- yes --> C[set job_type INSAR_ISCE_SINGLE]
B -- no --> D{matches INSAR_ISCE_MULTI_BURST?}
D -- yes --> E[set job_type INSAR_ISCE_MULTI_BURST]
D -- no --> F{matches S1ABCD_INSAR_GAMMA_pattern?}
F -- yes --> G[set job_type INSAR_GAMMA]
F -- no --> H[raise ValueError]
subgraph S1ABCD_INSAR_GAMMA_pattern
I("pattern S1[ABCD]{2}(_\d{8}T\d{6}){2}_(VV|HH)[PRO]\d{3,4}_INT\d{2}_G_[uw][ec][123F]_[0-9A-F]{4}")
end
F -. uses .-> I
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The updated regex now allows
Din both character positions afterS1(i.e.,S1[ABCD]{2}), but only the first character represents the platform while the second encodes mode/beam; consider tightening this to the actual allowed values for the second character or adding a brief comment explaining whyDis valid there as well. - Since INSAR_GAMMA filenames can have suffixes like
_unw_phase_clip.tif, it might be clearer to explicitly anchor the regex at the start (^) and rely on the absence of$to document that only the prefix is validated rather than the full filename.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The updated regex now allows `D` in both character positions after `S1` (i.e., `S1[ABCD]{2}`), but only the first character represents the platform while the second encodes mode/beam; consider tightening this to the actual allowed values for the second character or adding a brief comment explaining why `D` is valid there as well.
- Since INSAR_GAMMA filenames can have suffixes like `_unw_phase_clip.tif`, it might be clearer to explicitly anchor the regex at the start (`^`) and rely on the absence of `$` to document that only the prefix is validated rather than the full filename.
## Individual Comments
### Comment 1
<location path="tests/test_prep_hyp3.py" line_range="23-28" />
<code_context>
'S1BC_20210807T053645_20250729T053630_VVP1452_INT80_G_weF_9C74_unw_phase_clip.tif'
) == ('S1BC_20210807T053645_20250729T053630_VVP1452_INT80_G_weF_9C74', 'INSAR_GAMMA')
+ assert _get_product_name_and_type(
+ 'S1CD_20260605T013230_20260606T013240_VVR001_INT80_G_ueF_66F3_foo.tif'
+ ) == ('S1CD_20260605T013230_20260606T013240_VVR001_INT80_G_ueF_66F3', 'INSAR_GAMMA')
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a regression test that matches the originally failing S1DD *_unw_phase*.tif filename
The original failure was for `S1DD_*_unw_phase*.tif`, but this test uses an `S1CD_...foo.tif` name. Please add a test case with a realistic S1D unw phase filename (e.g. `S1DD_..._unw_phase_clip.tif` or `..._unw_phase.tif`, matching actual HyP3 output) and assert that `_get_product_name_and_type` returns the correct product name and `INSAR_GAMMA` type for it.
```suggestion
assert _get_product_name_and_type(
'S1CD_20260605T013230_20260606T013240_VVR001_INT80_G_ueF_66F3_foo.tif'
) == ('S1CD_20260605T013230_20260606T013240_VVR001_INT80_G_ueF_66F3', 'INSAR_GAMMA')
# Regression test for original S1DD unw_phase failure (HyP3-style filename)
assert _get_product_name_and_type(
'S1DD_20210807T053645_20250729T053630_VVP1452_INT80_G_weF_9C74_unw_phase_clip.tif'
) == ('S1DD_20210807T053645_20250729T053630_VVP1452_INT80_G_weF_9C74', 'INSAR_GAMMA')
# Old INSAR_ISCE_MULTI_BURST naming convention
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Description of proposed changes
Currently, attempting to load a HyP3 INSAR_GAMMA interferogram generated using a reference and/or secondary Sentinel-1D SLC causes a
ValueError: Failed to parse product name from filename: S1DD_*_unw_phase.tifThis PR adds Sentinel-1D support.
Reminders
Summary by Sourcery
Add support for parsing HyP3 INSAR_GAMMA products generated from Sentinel-1D data.
New Features:
Tests: