Closed
Conversation
…al.warp options to creationOptions, gdal.UseExceptions()
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAligns NISAR preparation script with updated product metadata and improves DEM path resolution and GDAL warp usage, while adding minimal debug output for geometry prep. Sequence diagram for updated DEM handling in prepare_geometrysequenceDiagram
actor User
participant prep_nisar as prep_nisar_module
participant prepare_geometry
participant read_subset
participant read_and_interpolate_geometry
participant GDAL as gdal
User->>prep_nisar: run script with demFile and input_glob
prep_nisar->>prepare_geometry: prepare_geometry(metaFile, demFile, ...)
prepare_geometry->>read_subset: read_subset(metaFile, bbox, geometry=True)
read_subset-->>prepare_geometry: geo_ds (includes xybbox)
prepare_geometry->>read_and_interpolate_geometry: read_and_interpolate_geometry(metaFile, demFile, geo_ds[xybbox], mask_file)
read_and_interpolate_geometry->>prep_nisar: Path.cwd()
read_and_interpolate_geometry->>prep_nisar: Path.cwd().rglob(demFile)
prep_nisar-->>read_and_interpolate_geometry: dem_path (absolute)
read_and_interpolate_geometry->>GDAL: Open(dem_path, GA_ReadOnly)
GDAL-->>read_and_interpolate_geometry: dem_dataset (projection, EPSG)
read_and_interpolate_geometry->>GDAL: Warp(output_dem, dem_path, outputBounds=bounds, format=GTiff, srcSRS=input_projection, dstSRS=output_projection, resampleAlg=GRA_Bilinear, width=subset_cols, height=subset_rows, creationOptions=[COMPRESS=DEFLATE])
read_and_interpolate_geometry->>GDAL: Open(output_dem, GA_ReadOnly).ReadAsArray()
GDAL-->>read_and_interpolate_geometry: dem_subset_array
read_and_interpolate_geometry-->>prepare_geometry: dem_subset_array, slant_range, incidence_angle, mask
prepare_geometry-->>prep_nisar: geometry datasets
prep_nisar-->>User: prepared NISAR geometry products
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The DEM resolution logic
dem_path = list(Path.cwd().rglob(dem_file))[0].resolve()is brittle and potentially expensive (no match, multiple matches, large trees); consider respecting the provided path first and only falling back to a controlled search with explicit error handling when the file is not found. - The new
printstatements for debugging (e.g.,inps.input_glob,Path.cwd(),length) should be replaced with the existing logging/verbosity mechanism or removed to avoid noisy stdout in normal runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The DEM resolution logic `dem_path = list(Path.cwd().rglob(dem_file))[0].resolve()` is brittle and potentially expensive (no match, multiple matches, large trees); consider respecting the provided path first and only falling back to a controlled search with explicit error handling when the file is not found.
- The new `print` statements for debugging (e.g., `inps.input_glob`, `Path.cwd()`, `length`) should be replaced with the existing logging/verbosity mechanism or removed to avoid noisy stdout in normal runs.
## Individual Comments
### Comment 1
<location path="src/mintpy/prep_nisar.py" line_range="62" />
<code_context>
print(f'update mode: {inps.update_mode}')
input_files = sorted(glob.glob(inps.input_glob))
+ print(inps.input_glob)
print(f"Found {len(input_files)} unwrapped files")
</code_context>
<issue_to_address>
**suggestion:** Avoid unconditional debug `print` in `load_nisar`, prefer logging or remove it.
This call will run on every execution and may clutter stdout or expose sensitive path info. If it’s only for debugging, please remove it or send it through the logger at debug level instead.
Suggested implementation:
```python
input_files = sorted(glob.glob(inps.input_glob))
logger.debug("Input glob pattern: %s", inps.input_glob)
print(f"Found {len(input_files)} unwrapped files")
```
If `logger` is not already defined in `prep_nisar.py`, you should also:
1. Add `import logging` near the top of the file.
2. Define a module-level logger, e.g.:
```python
logger = logging.getLogger(__name__)
```
This will ensure the `logger.debug(...)` call works correctly and respects the configured logging level.
</issue_to_address>
### Comment 2
<location path="src/mintpy/prep_nisar.py" line_range="251-252" />
<code_context>
"""Read the DEM and mask, change projection and interpolate to data grid,
interpolate slant range and incidence angle to data grid"""
- dem_dataset = gdal.Open(dem_file, gdal.GA_ReadOnly)
+ dem_path = list(Path.cwd().rglob(dem_file))[0].resolve()
+ dem_dataset = gdal.Open(dem_path, gdal.GA_ReadOnly)
proj = gdal.osr.SpatialReference(wkt=dem_dataset.GetProjection())
dem_src_epsg = int(proj.GetAttrValue('AUTHORITY', 1))
</code_context>
<issue_to_address>
**issue:** Using `Path.cwd().rglob(dem_file)[0]` is brittle and can fail or be slow; consider resolving the provided path directly.
This has two problems: it raises an unhelpful `IndexError` when no match is found, and it may be slow and pick an unintended file if multiple matches exist. If `dem_file` is already a path or relative to a known base, prefer resolving it directly, e.g. `dem_path = Path(dem_file).expanduser().resolve()` (or joining with an explicit base dir), then explicitly check it exists and raise a clear error if not.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+251
to
+252
| dem_path = list(Path.cwd().rglob(dem_file))[0].resolve() | ||
| dem_dataset = gdal.Open(dem_path, gdal.GA_ReadOnly) |
Contributor
There was a problem hiding this comment.
issue: Using Path.cwd().rglob(dem_file)[0] is brittle and can fail or be slow; consider resolving the provided path directly.
This has two problems: it raises an unhelpful IndexError when no match is found, and it may be slow and pick an unintended file if multiple matches exist. If dem_file is already a path or relative to a known base, prefer resolving it directly, e.g. dem_path = Path(dem_file).expanduser().resolve() (or joining with an explicit base dir), then explicitly check it exists and raise a clear error if not.
Member
|
@Alex-Lewandowski Looks good to me in general, could you fix a few minor issues flagged by pre-comit and sourcery-ai? Normally, I would fix these minor issues myself, but it seems that I do not have permission to modify the PR changes here. |
Contributor
Author
Hi @yunjunz, Yes, I just started this as a draft PR and still plan to work on it a bit more. I'd like to add tests now that some NISAR data has been released.
I bet that's because I submitted the PR from a fork. I can move it into a branch if you'd like access. |
Member
|
Sounds good @Alex-Lewandowski, take your time.
Strangely, I could modify other PRs from forks but not this one. Not a big deal, as long as quality checks are all passed. :) |
Contributor
|
@Alex-Lewandowski @yunjunz |
Member
Thank you for letting us know @ehavazli. Please consider issuing a PR whenever it's ready. |
Contributor
Author
|
Closing in favor of this more comprehensive PR: #1487 |
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
3 participants
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
Reminders
Summary by Sourcery
Update NISAR preparation to use the correct reference slant range attribute and more robust DEM file handling while improving GDAL usage and logging.
Enhancements: