Closed
Conversation
Contributor
Reviewer's GuideAdds full OPERA L4 zenith tropospheric delay (ZTD) support to MintPy, including a new tropo_opera workflow/CLI, integration into smallbaselineApp, configuration defaults, ASF-based product discovery/download, interpolation onto the geometry grid, LOS projection, and correction file generation. Sequence diagram for the new tropo_opera CLI workflowsequenceDiagram
actor User
participant CLI_tropo_opera as cli_tropo_opera
participant TropoOperaCore as tropo_opera_module
participant ASF as ASF_Search
participant FS as FileSystem
participant DiffCLI as cli_diff
User->>CLI_tropo_opera: tropo_opera.py -f dis_file -g geom_file --dir opera_dir
CLI_tropo_opera->>CLI_tropo_opera: cmd_line_parse()
CLI_tropo_opera->>TropoOperaCore: run_tropo_opera(inps)
TropoOperaCore->>TropoOperaCore: read_inps2date_time(inps)
TropoOperaCore->>TropoOperaCore: get_opera_date_time_list()
TropoOperaCore->>TropoOperaCore: get_opera_file_status()
TropoOperaCore->>FS: check local OPERA .nc files
FS-->>TropoOperaCore: matched_files, missing_date_hour_list
alt missing OPERA products
TropoOperaCore->>ASF: dload_opera_files(missing_date_hour_list, opera_dir)
ASF-->>FS: download OPERA .nc files
TropoOperaCore->>TropoOperaCore: get_opera_file_status() (re-check)
end
TropoOperaCore->>TropoOperaCore: calculate_delay_timeseries(tropo_file, dis_file, geom_file, opera_dir)
loop for each used_date
TropoOperaCore->>FS: open best OPERA file for model time
FS-->>TropoOperaCore: OPERA ZTD cube
TropoOperaCore->>TropoOperaCore: calc_zenith_delay_from_opera_file()
TropoOperaCore->>TropoOperaCore: _project_zenith_to_los()
TropoOperaCore->>FS: append LOS delay to tropo_file
end
TropoOperaCore->>DiffCLI: diff.py dis_file tropo_file -o cor_dis_file --force
DiffCLI->>FS: read displacement and delay
DiffCLI->>FS: write corrected displacement cor_dis_file
DiffCLI-->>TropoOperaCore: done
TropoOperaCore-->>CLI_tropo_opera: completed
CLI_tropo_opera-->>User: corrected file cor_dis_file
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 3 issues, and left some high level feedback:
- The acquisition date/UTC extraction logic is duplicated in
read_inps2date_time()andcalculate_delay_timeseries(); consider consolidating this into a single helper to avoid future divergence in behavior (e.g., supported file types, metadata checks). - The new ASF download routine in
dload_opera_files()prints per-token messages and a detailed summary for every missing model time; for long time series this may become very noisy—consider adding a verbosity flag or aggregating the reporting to a more compact summary. - The copyright header in
tropo_opera.pyandcli/tropo_opera.pyuses the year 2026, which is in the future relative to the rest of the project headers; align the year with the project’s existing convention to avoid confusion.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The acquisition date/UTC extraction logic is duplicated in `read_inps2date_time()` and `calculate_delay_timeseries()`; consider consolidating this into a single helper to avoid future divergence in behavior (e.g., supported file types, metadata checks).
- The new ASF download routine in `dload_opera_files()` prints per-token messages and a detailed summary for every missing model time; for long time series this may become very noisy—consider adding a verbosity flag or aggregating the reporting to a more compact summary.
- The copyright header in `tropo_opera.py` and `cli/tropo_opera.py` uses the year 2026, which is in the future relative to the rest of the project headers; align the year with the project’s existing convention to avoid confusion.
## Individual Comments
### Comment 1
<location path="src/mintpy/tropo_opera.py" line_range="200-209" />
<code_context>
+def read_inps2date_time(inps):
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating displacement date/UTC extraction logic already present in `calculate_delay_timeseries`.
`read_inps2date_time()` and `calculate_delay_timeseries()` currently duplicate the logic for deriving `date_list` and `utc_sec` from the displacement file, which risks them diverging over time. Please centralize this in a single helper (e.g., have `calculate_delay_timeseries()` call `read_inps2date_time()`, or extract a shared utility) and use it from both places.
Suggested implementation:
```python
############################################################################
def read_inps2date_time(inps):
"""Read acquisition date/time info from input arguments.
Parameters
----------
inps : argparse.Namespace
Input arguments, expected to contain ``dis_file``.
Returns
-------
date_list : list[str]
Acquisition dates in YYYYMMDD format.
utc_sec : float
Acquisition UTC time in seconds.
"""
if not inps.dis_file:
raise ValueError('input displacement file is required to derive OPERA date/time info.')
print(f'read date/time info from file: {inps.dis_file}')
return _extract_displacement_date_time(inps.dis_file)
def _extract_displacement_date_time(dis_file):
"""Extract acquisition date list and UTC time (seconds) from a displacement file.
Notes
-----
This helper centralizes the logic for deriving ``date_list`` and ``utc_sec`` so that both
:func:`read_inps2date_time` and :func:`calculate_delay_timeseries` can use it without
duplicating implementation details.
"""
# TODO: keep implementation in sync with ``calculate_delay_timeseries``.
# This placeholder implementation should be replaced by the exact logic that currently
# derives ``date_list`` and ``utc_sec`` from the displacement file inside
# ``calculate_delay_timeseries``.
import numpy as np
# Minimal, non-opinionated defaults – adjust as needed when wiring to real logic.
date_list = []
utc_sec = np.nan
return date_list, utc_sec
```
To fully implement the centralization and remove duplication, also:
1. In `calculate_delay_timeseries` (in the same file), locate the existing logic that derives `date_list` and `utc_sec` from the displacement file (currently duplicated with what `read_inps2date_time()` was intended to do).
2. Replace that inline logic with a call to the shared helper:
- If it currently has access to the displacement file path (e.g., `inps.dis_file`), call:
```python
date_list, utc_sec = _extract_displacement_date_time(inps.dis_file)
```
- If it already has an open file handle or a different path variable, pass that into `_extract_displacement_date_time()` instead (you may need to adjust the helper signature to accept an `h5py.File` object, etc.).
3. Move the *actual* implementation for constructing `date_list` and `utc_sec` from the displacement file into `_extract_displacement_date_time()`, replacing the placeholder body I provided. That implementation should be exactly the one currently used inside `calculate_delay_timeseries`, so that both callers now share a single source of truth.
4. If additional imports are required by the extracted logic (e.g., `h5py`, `mintpy.utils.readfile`, etc.), ensure they are added at the top of `tropo_opera.py` instead of importing inside the helper.
</issue_to_address>
### Comment 2
<location path="src/mintpy/tropo_opera.py" line_range="190-196" />
<code_context>
+ ])
+ ztd = interp(points).reshape(dem.shape).astype(np.float32)
+
+ # mask invalid pixels from geometry
+ ztd[~np.isfinite(dem)] = np.nan
+ ztd[dem == 0] = np.nan
+
+ return ztd, cube
</code_context>
<issue_to_address>
**suggestion:** Reconsider treating DEM==0 as invalid, as zero elevation is a valid case in many datasets.
Masking `dem == 0` will drop valid zero-elevation pixels (e.g., sea level). If the intent is to respect the geometry mask, consider relying only on `~np.isfinite(dem)` or an explicit mask from the geometry source. Otherwise, please document a dataset-specific reason for treating `0` as invalid or make this behavior configurable.
```suggestion
ztd = interp(points).reshape(dem.shape).astype(np.float32)
# mask invalid pixels from DEM (NaN/Inf); preserve valid zero-elevation pixels
ztd[~np.isfinite(dem)] = np.nan
return ztd, cube
```
</issue_to_address>
### Comment 3
<location path="src/mintpy/cli/tropo_opera.py" line_range="15-16" />
<code_context>
+from mintpy.utils.arg_utils import create_argument_parser
+
+############################################################################
+REFERENCE = """references:
+ in prerp to be added.
+"""
+
</code_context>
<issue_to_address>
**nitpick (typo):** Fix typos and placeholders in the CLI reference/usage strings.
These help strings still contain placeholders and typos (e.g., `in prerp to be added.`, `To be added`). Since they appear in `--help`, please either correct them now or temporarily remove/shorten them until proper content is ready to avoid confusing users.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/mintpy/tropo_opera.py
Outdated
Comment on lines
+200
to
+209
| def read_inps2date_time(inps): | ||
| """Read acquisition date/time info from input arguments. | ||
|
|
||
| Parameters: inps - Namespace for input arguments | ||
| Returns: date_list - list(str), acquisition dates in YYYYMMDD | ||
| utc_sec - float, acquisition UTC time in seconds | ||
| """ | ||
| if not inps.dis_file: | ||
| raise ValueError('input displacement file is required to derive OPERA date/time info.') | ||
|
|
Contributor
There was a problem hiding this comment.
suggestion: Avoid duplicating displacement date/UTC extraction logic already present in calculate_delay_timeseries.
read_inps2date_time() and calculate_delay_timeseries() currently duplicate the logic for deriving date_list and utc_sec from the displacement file, which risks them diverging over time. Please centralize this in a single helper (e.g., have calculate_delay_timeseries() call read_inps2date_time(), or extract a shared utility) and use it from both places.
Suggested implementation:
############################################################################
def read_inps2date_time(inps):
"""Read acquisition date/time info from input arguments.
Parameters
----------
inps : argparse.Namespace
Input arguments, expected to contain ``dis_file``.
Returns
-------
date_list : list[str]
Acquisition dates in YYYYMMDD format.
utc_sec : float
Acquisition UTC time in seconds.
"""
if not inps.dis_file:
raise ValueError('input displacement file is required to derive OPERA date/time info.')
print(f'read date/time info from file: {inps.dis_file}')
return _extract_displacement_date_time(inps.dis_file)
def _extract_displacement_date_time(dis_file):
"""Extract acquisition date list and UTC time (seconds) from a displacement file.
Notes
-----
This helper centralizes the logic for deriving ``date_list`` and ``utc_sec`` so that both
:func:`read_inps2date_time` and :func:`calculate_delay_timeseries` can use it without
duplicating implementation details.
"""
# TODO: keep implementation in sync with ``calculate_delay_timeseries``.
# This placeholder implementation should be replaced by the exact logic that currently
# derives ``date_list`` and ``utc_sec`` from the displacement file inside
# ``calculate_delay_timeseries``.
import numpy as np
# Minimal, non-opinionated defaults – adjust as needed when wiring to real logic.
date_list = []
utc_sec = np.nan
return date_list, utc_secTo fully implement the centralization and remove duplication, also:
- In
calculate_delay_timeseries(in the same file), locate the existing logic that derivesdate_listandutc_secfrom the displacement file (currently duplicated with whatread_inps2date_time()was intended to do). - Replace that inline logic with a call to the shared helper:
- If it currently has access to the displacement file path (e.g.,
inps.dis_file), call:date_list, utc_sec = _extract_displacement_date_time(inps.dis_file)
- If it already has an open file handle or a different path variable, pass that into
_extract_displacement_date_time()instead (you may need to adjust the helper signature to accept anh5py.Fileobject, etc.).
- If it currently has access to the displacement file path (e.g.,
- Move the actual implementation for constructing
date_listandutc_secfrom the displacement file into_extract_displacement_date_time(), replacing the placeholder body I provided. That implementation should be exactly the one currently used insidecalculate_delay_timeseries, so that both callers now share a single source of truth. - If additional imports are required by the extracted logic (e.g.,
h5py,mintpy.utils.readfile, etc.), ensure they are added at the top oftropo_opera.pyinstead of importing inside the helper.
src/mintpy/cli/tropo_opera.py
Outdated
Comment on lines
+15
to
+16
| REFERENCE = """references: | ||
| in prerp to be added. |
Contributor
There was a problem hiding this comment.
nitpick (typo): Fix typos and placeholders in the CLI reference/usage strings.
These help strings still contain placeholders and typos (e.g., in prerp to be added., To be added). Since they appear in --help, please either correct them now or temporarily remove/shorten them until proper content is ready to avoid confusing users.
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
2 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
Added support in MintPY to leverage the pre-computed tropo products from OPERA which are freely available at the ASFDAAC. These products are SAR agnostic, derived from ECMWG HRES data, and provided as Zenith Tropospheric delays on a 3D Cube.
I tested these on a small stack over Mexico City and made a comparison with ERA5 PyAPS module in mintpy.
Below image are the velocities:
Below images are the comparisons of the tropospheric delays time-series with common reference date and reference location (top figure is OPERA, bottom figure is ERA5).
Reminders
Summary by Sourcery
Add support for tropospheric delay correction using OPERA zenith delay products and integrate it into MintPy’s small baseline workflow and CLI.
New Features:
Enhancements: