Modified asc_desc2horz_vert.py to specify different dataset for each input files #1407
Conversation
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnable per-file dataset selection by converting the --dset/ ds_name parameter to a list, updating the CLI to accept multiple dataset names, and adjusting internal logic to read attributes and LOS data using the corresponding dataset entry for each input file. Class diagram for updated input parameter handling in asc_desc2horz_vert.pyclassDiagram
class Inps {
file: list[str]
ds_name: list[str] // now supports multiple dataset names
geom_file: list[str]
}
class run_asc_desc2horz_vert {
+run_asc_desc2horz_vert(inps: Inps)
}
Inps <.. run_asc_desc2horz_vert: uses
Flow diagram for per-file dataset selection in asc_desc2horz_vert.pyflowchart TD
A[Start] --> B[Parse CLI arguments]
B --> C{Are multiple datasets specified?}
C -- Yes --> D[Assign each ds_name to corresponding file]
C -- No --> E[Use single ds_name for both files]
D --> F[Read attributes and data per file/dataset]
E --> F
F --> G[Continue processing]
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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Simplify the dataset selection logic by directly using the loop index to pick ds_name[i] (or default to ds_name[0]) instead of the nested range and inps.file.index calls.
- Normalize ds_name at the beginning (e.g., wrap a single string into a list matching the number of input files) and guard against it being None to avoid len(None) errors.
- Ensure the same per-file dataset selection is applied when reading los_inc_angle and los_az_angle to keep the logic consistent across all readfile calls.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Simplify the dataset selection logic by directly using the loop index to pick ds_name[i] (or default to ds_name[0]) instead of the nested range and inps.file.index calls.
- Normalize ds_name at the beginning (e.g., wrap a single string into a list matching the number of input files) and guard against it being None to avoid len(None) errors.
- Ensure the same per-file dataset selection is applied when reading los_inc_angle and los_az_angle to keep the logic consistent across all readfile calls.
## Individual Comments
### Comment 1
<location> `src/mintpy/asc_desc2horz_vert.py:167` </location>
<code_context>
## 1. calculate the overlapping area in lat/lon
- atr_list = [readfile.read_attribute(fname, datasetName=inps.ds_name) for fname in inps.file]
+ atr_list = [readfile.read_attribute(fname, datasetName=inps.ds_name[0]) for fname in inps.file]
S, N, W, E = get_overlap_lalo(atr_list)
lat_step = float(atr_list[0]['Y_STEP'])
</code_context>
<issue_to_address>
Hardcoding ds_name[0] may not generalize for multiple datasets.
Iterate over inps.file and inps.ds_name together to match each file with its corresponding dataset name.
</issue_to_address>
### Comment 2
<location> `src/mintpy/asc_desc2horz_vert.py:194` </location>
<code_context>
# read data
- dlos[i, :] = readfile.read(fname, box=box, datasetName=inps.ds_name)[0]
+ if dataset_order > 1:
+ for i in range(dataset_order):
+ if inps.file.index(fname) == i:
+ dlos[i, :] = readfile.read(fname, box=box, datasetName=inps.ds_name[i])[0]
</code_context>
<issue_to_address>
Nested loop with index reuse may cause logic errors.
Using 'i' for both loops can cause variable shadowing and unintended behavior. Please rename the inner loop variable to prevent this issue.
</issue_to_address>
### Comment 3
<location> `src/mintpy/cli/asc_desc2horz_vert.py:60` </location>
<code_context>
help='Ascending and descending files\n'
'Both files need to be geocoded in the same spatial resolution.')
- parser.add_argument('-d', '--dset', dest='ds_name', type=str, help='dataset to use, default: 1st dataset')
+ parser.add_argument('-d', '--dset', dest='ds_name', type=str, nargs='+', help='dataset to use, default: 1st dataset')
parser.add_argument('-g','--geom-file', dest='geom_file', nargs=2, help='Geometry files for the input data files.')
</code_context>
<issue_to_address>
Changing ds_name to accept multiple values may require validation.
Please ensure the number of dataset names matches the number of geometry files to prevent mismatches.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| ## 1. calculate the overlapping area in lat/lon | ||
| atr_list = [readfile.read_attribute(fname, datasetName=inps.ds_name) for fname in inps.file] | ||
| atr_list = [readfile.read_attribute(fname, datasetName=inps.ds_name[0]) for fname in inps.file] |
There was a problem hiding this comment.
issue: Hardcoding ds_name[0] may not generalize for multiple datasets.
Iterate over inps.file and inps.ds_name together to match each file with its corresponding dataset name.
Comment on lines
+194
to
+195
| if dataset_order > 1: | ||
| for i in range(dataset_order): |
There was a problem hiding this comment.
issue (bug_risk): Nested loop with index reuse may cause logic errors.
Using 'i' for both loops can cause variable shadowing and unintended behavior. Please rename the inner loop variable to prevent this issue.
| help='Ascending and descending files\n' | ||
| 'Both files need to be geocoded in the same spatial resolution.') | ||
| parser.add_argument('-d', '--dset', dest='ds_name', type=str, help='dataset to use, default: 1st dataset') | ||
| parser.add_argument('-d', '--dset', dest='ds_name', type=str, nargs='+', help='dataset to use, default: 1st dataset') |
There was a problem hiding this comment.
suggestion: Changing ds_name to accept multiple values may require validation.
Please ensure the number of dataset names matches the number of geometry files to prevent mismatches.
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
Modified asc_desc2horz_vert.py to specify different dataset other than the same dataset for the two input files when decomposing to horizontal and vertical displacement/velocity.
Reminders
Summary by Sourcery
Allow users to specify a different dataset name for each input file when converting ascending/descending measurements to horizontal and vertical components
New Features:
Enhancements: