plot_coherence_matrix: add continuous time-axis view#1495
Open
huchangyang wants to merge 1 commit into
Open
Conversation
Contributor
Reviewer's GuideAdds a new continuous time-axis coherence matrix plot function and integrates it into both the interactive coherence-matrix viewer and the plot_network workflow, including a new saved figure and a CLI flag to toggle the new view. Flow diagram for coherence matrix plotting with continuous time-axis optionflowchart LR
subgraph Interactive_viewer
CLI_plot_coherence_matrix["cli/plot_coherence_matrix.py
parse args (including --time-axis)"]
PlotCoherenceMatrixClass["plot_coherence_matrix.CoherenceMatrixPlotter"]
Method_plot_coh["plot_coherence_matrix.plot_coherence_matrix4pixel"]
Method_plot_coh_time["plot_coherence_matrix.plot_coherence_matrix4pixel_time_axis"]
end
subgraph Core_plotting
Util_plot_time_axis["mintpy.utils.plot.plot_coherence_matrix_time_axis"]
end
subgraph Network_workflow
Plot_network["plot_network.plot_network"]
Fig4_time_axis["Fig 4: coherenceMatrixTimeAxis.pdf"]
Fig5_network["Fig 5: network (or Fig 4 if cohList is None)"]
end
CLI_plot_coherence_matrix --> PlotCoherenceMatrixClass
PlotCoherenceMatrixClass -->|time_axis == False| Method_plot_coh
PlotCoherenceMatrixClass -->|time_axis == True| Method_plot_coh_time
Method_plot_coh --> Util_plot_time_axis
Method_plot_coh_time --> Util_plot_time_axis
Plot_network -->|cohList is not None| Util_plot_time_axis
Util_plot_time_axis -->|pcolormesh on date grid| Fig4_time_axis
Plot_network --> Fig5_network
File-Level Changes
Possibly linked issues
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 4 issues, and left some high level feedback:
- The legend entries and annotations for the time-axis coherence matrix still refer to "upper = used / lower = all" interferograms, but this implementation only uses a single matrix with dropped pairs set to NaN, so the legend text is misleading and should be updated or removed where enabled (e.g., in plot_network).
- plot_coherence_matrix_time_axis duplicates quite a bit of setup/formatting logic from plot_coherence_matrix; consider factoring out common pieces (e.g., default p_dict handling, colormap and colorbar setup) into a shared helper to reduce divergence and make future changes easier.
- In plot_network, you pass vars(inps) directly as p_dict into plot_coherence_matrix_time_axis; if inps gains additional attributes with names overlapping the expected keys (e.g., colormap, vlim, legend_loc), they will silently override the intended defaults, so it may be safer to build a smaller dict with only the keys that plot_coherence_matrix_time_axis uses.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The legend entries and annotations for the time-axis coherence matrix still refer to "upper = used / lower = all" interferograms, but this implementation only uses a single matrix with dropped pairs set to NaN, so the legend text is misleading and should be updated or removed where enabled (e.g., in plot_network).
- plot_coherence_matrix_time_axis duplicates quite a bit of setup/formatting logic from plot_coherence_matrix; consider factoring out common pieces (e.g., default p_dict handling, colormap and colorbar setup) into a shared helper to reduce divergence and make future changes easier.
- In plot_network, you pass vars(inps) directly as p_dict into plot_coherence_matrix_time_axis; if inps gains additional attributes with names overlapping the expected keys (e.g., colormap, vlim, legend_loc), they will silently override the intended defaults, so it may be safer to build a smaller dict with only the keys that plot_coherence_matrix_time_axis uses.
## Individual Comments
### Comment 1
<location path="src/mintpy/utils/plot.py" line_range="962" />
<code_context>
return ax, coh_mat, im
+def plot_coherence_matrix_time_axis(ax, date12List, cohList, date12List_drop=[], p_dict={}):
+ """Plot Coherence Matrix with continuous time axis
+ Parameters: ax : matplotlib.pyplot.Axes,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid using a mutable dict as the default value for p_dict.
`{}` as a default argument makes all calls share the same dict instance, which can cause subtle bugs if `p_dict` is mutated. Prefer `p_dict=None` and then inside: `p_dict = {} if p_dict is None else dict(p_dict)`.
</issue_to_address>
### Comment 2
<location path="src/mintpy/utils/plot.py" line_range="1001-1005" />
<code_context>
+ date12List_drop = ptime.yyyymmdd_date12(date12List_drop)
+ for date12 in date12List_drop:
+ idx1, idx2 = (dateList.index(i) for i in date12.split('_'))
+ coh_mat[idx1, idx2] = np.nan
+
+ if len(dateList_dt) == 1:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Dropped interferograms are only masked on one triangle of the symmetric matrix.
Because `coherence_matrix` is symmetric, updating only `coh_mat[idx1, idx2]` leaves `coh_mat[idx2, idx1]` unmasked, which can produce inconsistent plots. Please mask both `[idx1, idx2]` and `[idx2, idx1]` to keep the visualization symmetric.
```suggestion
if date12List_drop:
date12List_drop = ptime.yyyymmdd_date12(date12List_drop)
for date12 in date12List_drop:
idx1, idx2 = (dateList.index(i) for i in date12.split('_'))
coh_mat[idx1, idx2] = np.nan
coh_mat[idx2, idx1] = np.nan
```
</issue_to_address>
### Comment 3
<location path="src/mintpy/utils/plot.py" line_range="1092" />
<code_context>
+ ax.set_xlabel('Time', fontsize=fs, labelpad=18)
+ ax.set_ylabel('Time', fontsize=fs, labelpad=18)
+ ax.set_aspect('equal', adjustable='box')
+ ax.invert_yaxis()
+
+ if p_dict['disp_title']:
</code_context>
<issue_to_address>
**issue (bug_risk):** Inverting the y-axis after placing month/year labels likely moves them away from the intended edges.
Because the month/year positions use `ax.get_ylim()[1]` and `ax.get_xlim()[0]` computed before `invert_yaxis()`, those values no longer map to the top/left edges after inversion, so the labels can shift toward the bottom or misalign. Either invert the y-axis before computing `offset`/placing the labels, or place them in axis coordinates (`transform=ax.transAxes`) so they stay fixed to the edges regardless of axis direction.
</issue_to_address>
### Comment 4
<location path="src/mintpy/plot_coherence_matrix.py" line_range="221" />
<code_context>
+ self.fig_img.canvas.draw_idle()
+ return
+
def plot_coherence_matrix4pixel(self, yx):
"""Plot coherence matrix for one pixel
Parameters: yx : list of 2 int
</code_context>
<issue_to_address>
**issue (complexity):** Consider unifying the two pixel plotting paths into a single parameterized `plot_coherence_matrix4pixel` function that selects the appropriate plotting routine based on `time_axis` while sharing all common setup and marker logic.
You can remove most of the duplication by parameterizing the plot function and keeping a single `plot_coherence_matrix4pixel` implementation. That also lets you unify the layout and marker logic.
For example:
```python
def plot_coherence_matrix4pixel(self, yx):
"""Plot coherence matrix for one pixel."""
# choose plotting function based on time_axis
plot_func = (
pp.plot_coherence_matrix_time_axis
if self.time_axis
else pp.plot_coherence_matrix
)
self.ax_mat.cla()
# read coherence
box = (yx[1], yx[0], yx[1] + 1, yx[0] + 1)
coh = readfile.read(self.ifgram_file, datasetName='coherence', box=box)[0]
# ex_date for pixel-wise masking during network inversion
ex_date12_list = self.ex_date12_list[:] # local copy
if self.min_coh_used > 0.0:
ex_date12_list += np.array(self.date12_list)[coh < self.min_coh_used].tolist()
ex_date12_list = sorted(set(ex_date12_list))
# common plot config
plotDict = {
"fig_title": f"Y = {yx[0]}, X = {yx[1]}",
"colormap": self.colormap,
"cbar_label": "Coherence",
"disp_legend": False,
}
if self.tcoh is not None:
tcoh = self.tcoh[yx[0], yx[1]]
plotDict["fig_title"] += f", tcoh = {tcoh:.2f}"
if len(self.cmap_vlist) >= 2:
plotDict["vlim"] = [self.cmap_vlist[0], self.cmap_vlist[-1]]
else:
plotDict["vlim"] = [0.0, 1.0]
# call the selected plotting function
plot_func(
self.ax_mat,
date12List=self.date12_list,
cohList=coh.tolist(),
date12List_drop=ex_date12_list,
p_dict=plotDict,
)
# common annotations
self.ax_mat.annotate(
"ifgrams\navailable",
xy=(0.05, 0.05),
xycoords="axes fraction",
fontsize=12,
)
self.ax_mat.annotate(
"ifgrams\nused",
ha="right",
xy=(0.95, 0.85),
xycoords="axes fraction",
fontsize=12,
)
msg = f"pixel in yx = {tuple(yx)}, "
msg += (
f"min/max spatial coherence: {np.nanmin(coh):.2f} / {np.nanmax(coh):.2f}, "
)
if self.tcoh is not None:
msg += f"temporal coherence: {tcoh:.2f}"
vprint(msg)
self.fig_mat.canvas.manager.set_window_title(self.figname_mat)
if not hasattr(self, "_mat_tight_layout_done"):
self.fig_mat.tight_layout()
self._mat_tight_layout_done = True
self.fig_mat.canvas.draw_idle()
self.fig_mat.canvas.flush_events()
# common marker logic
if self.fig_coord == "geo":
lat, lon = self.coord.yx2lalo(yx[0], yx[1])
mx, my = lon, lat
else:
mx, my = yx[1], yx[0]
if self._marker_artist is None:
self._marker_artist = self.ax_img.plot(
mx, my, "r^", markersize=6, markeredgecolor="black"
)[0]
else:
self._marker_artist.set_data([mx], [my])
self.fig_img.canvas.draw_idle()
```
Actionable steps:
1. Remove `plot_coherence_matrix4pixel_time_axis` entirely.
2. Fold its unique behavior into `plot_coherence_matrix4pixel` by:
- Selecting `plot_func` based on `self.time_axis`.
- Reusing shared logic for:
- reading `coh` / building `ex_date12_list`
- constructing `plotDict` (including `tcoh`)
- annotations, logging, and window title
- tight layout (`_mat_tight_layout_done`) for both paths
- marker update on `ax_img`.
3. Keep the existing public API (`plot_coherence_matrix4pixel` signature) and the `time_axis` flag, so no external behavior changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return ax, coh_mat, im | ||
|
|
||
|
|
||
| def plot_coherence_matrix_time_axis(ax, date12List, cohList, date12List_drop=[], p_dict={}): |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Avoid using a mutable dict as the default value for p_dict.
{} as a default argument makes all calls share the same dict instance, which can cause subtle bugs if p_dict is mutated. Prefer p_dict=None and then inside: p_dict = {} if p_dict is None else dict(p_dict).
Comment on lines
+1001
to
+1005
| if date12List_drop: | ||
| date12List_drop = ptime.yyyymmdd_date12(date12List_drop) | ||
| for date12 in date12List_drop: | ||
| idx1, idx2 = (dateList.index(i) for i in date12.split('_')) | ||
| coh_mat[idx1, idx2] = np.nan |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Dropped interferograms are only masked on one triangle of the symmetric matrix.
Because coherence_matrix is symmetric, updating only coh_mat[idx1, idx2] leaves coh_mat[idx2, idx1] unmasked, which can produce inconsistent plots. Please mask both [idx1, idx2] and [idx2, idx1] to keep the visualization symmetric.
Suggested change
| if date12List_drop: | |
| date12List_drop = ptime.yyyymmdd_date12(date12List_drop) | |
| for date12 in date12List_drop: | |
| idx1, idx2 = (dateList.index(i) for i in date12.split('_')) | |
| coh_mat[idx1, idx2] = np.nan | |
| if date12List_drop: | |
| date12List_drop = ptime.yyyymmdd_date12(date12List_drop) | |
| for date12 in date12List_drop: | |
| idx1, idx2 = (dateList.index(i) for i in date12.split('_')) | |
| coh_mat[idx1, idx2] = np.nan | |
| coh_mat[idx2, idx1] = np.nan |
| ax.set_xlabel('Time', fontsize=fs, labelpad=18) | ||
| ax.set_ylabel('Time', fontsize=fs, labelpad=18) | ||
| ax.set_aspect('equal', adjustable='box') | ||
| ax.invert_yaxis() |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Inverting the y-axis after placing month/year labels likely moves them away from the intended edges.
Because the month/year positions use ax.get_ylim()[1] and ax.get_xlim()[0] computed before invert_yaxis(), those values no longer map to the top/left edges after inversion, so the labels can shift toward the bottom or misalign. Either invert the y-axis before computing offset/placing the labels, or place them in axis coordinates (transform=ax.transAxes) so they stay fixed to the edges regardless of axis direction.
| self.fig_img.canvas.draw_idle() | ||
| return | ||
|
|
||
| def plot_coherence_matrix4pixel(self, yx): |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider unifying the two pixel plotting paths into a single parameterized plot_coherence_matrix4pixel function that selects the appropriate plotting routine based on time_axis while sharing all common setup and marker logic.
You can remove most of the duplication by parameterizing the plot function and keeping a single plot_coherence_matrix4pixel implementation. That also lets you unify the layout and marker logic.
For example:
def plot_coherence_matrix4pixel(self, yx):
"""Plot coherence matrix for one pixel."""
# choose plotting function based on time_axis
plot_func = (
pp.plot_coherence_matrix_time_axis
if self.time_axis
else pp.plot_coherence_matrix
)
self.ax_mat.cla()
# read coherence
box = (yx[1], yx[0], yx[1] + 1, yx[0] + 1)
coh = readfile.read(self.ifgram_file, datasetName='coherence', box=box)[0]
# ex_date for pixel-wise masking during network inversion
ex_date12_list = self.ex_date12_list[:] # local copy
if self.min_coh_used > 0.0:
ex_date12_list += np.array(self.date12_list)[coh < self.min_coh_used].tolist()
ex_date12_list = sorted(set(ex_date12_list))
# common plot config
plotDict = {
"fig_title": f"Y = {yx[0]}, X = {yx[1]}",
"colormap": self.colormap,
"cbar_label": "Coherence",
"disp_legend": False,
}
if self.tcoh is not None:
tcoh = self.tcoh[yx[0], yx[1]]
plotDict["fig_title"] += f", tcoh = {tcoh:.2f}"
if len(self.cmap_vlist) >= 2:
plotDict["vlim"] = [self.cmap_vlist[0], self.cmap_vlist[-1]]
else:
plotDict["vlim"] = [0.0, 1.0]
# call the selected plotting function
plot_func(
self.ax_mat,
date12List=self.date12_list,
cohList=coh.tolist(),
date12List_drop=ex_date12_list,
p_dict=plotDict,
)
# common annotations
self.ax_mat.annotate(
"ifgrams\navailable",
xy=(0.05, 0.05),
xycoords="axes fraction",
fontsize=12,
)
self.ax_mat.annotate(
"ifgrams\nused",
ha="right",
xy=(0.95, 0.85),
xycoords="axes fraction",
fontsize=12,
)
msg = f"pixel in yx = {tuple(yx)}, "
msg += (
f"min/max spatial coherence: {np.nanmin(coh):.2f} / {np.nanmax(coh):.2f}, "
)
if self.tcoh is not None:
msg += f"temporal coherence: {tcoh:.2f}"
vprint(msg)
self.fig_mat.canvas.manager.set_window_title(self.figname_mat)
if not hasattr(self, "_mat_tight_layout_done"):
self.fig_mat.tight_layout()
self._mat_tight_layout_done = True
self.fig_mat.canvas.draw_idle()
self.fig_mat.canvas.flush_events()
# common marker logic
if self.fig_coord == "geo":
lat, lon = self.coord.yx2lalo(yx[0], yx[1])
mx, my = lon, lat
else:
mx, my = yx[1], yx[0]
if self._marker_artist is None:
self._marker_artist = self.ax_img.plot(
mx, my, "r^", markersize=6, markeredgecolor="black"
)[0]
else:
self._marker_artist.set_data([mx], [my])
self.fig_img.canvas.draw_idle()Actionable steps:
- Remove
plot_coherence_matrix4pixel_time_axisentirely. - Fold its unique behavior into
plot_coherence_matrix4pixelby:- Selecting
plot_funcbased onself.time_axis. - Reusing shared logic for:
- reading
coh/ buildingex_date12_list - constructing
plotDict(includingtcoh) - annotations, logging, and window title
- tight layout (
_mat_tight_layout_done) for both paths - marker update on
ax_img.
- reading
- Selecting
- Keep the existing public API (
plot_coherence_matrix4pixelsignature) and thetime_axisflag, so no external behavior changes.
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
1 participant
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
This PR adds a continuous time-axis view for the per-pixel coherence matrix, complementing the existing date-index matrix from #1493.
Motivation: The index-based matrix uses acquisition order (0, 1, 2, …), which is hard to read when dates are irregular. A time-axis view maps each acquisition to its calendar date and uses date-centered cell edges (including extension at the first/last acquisition), matching the layout used in our analysis notebooks.
Changes:
mintpy.utils.plot.plot_coherence_matrix_time_axis()New plotting function (same pattern as
plot_coherence_matrix()):pcolormeshon a date grid, numeric month/year axis labels, diagonal masking, dropped-ifgram handling, colorbar, and status-barformat_coord.plot_network.pyAdds Figure 4 —
coherenceMatrixTimeAxis.pdf— when coherence lists are available. Fixesfig_namesindex for save/print messages when inserting this figure.plot_coherence_matrix.py(interactive)Adds
--time-axisto switch the matrix window to the continuous time-axis view; map window and click-to-update behavior are unchanged.cli/plot_coherence_matrix.pyRegisters the
--time-axisflag only (no change to default aux file paths: stillvelocity.h5/temporalCoherence.h5).Examples
Interactive viewer:
Figure 4
Reminders
Summary by Sourcery
Add a continuous time-axis coherence matrix view and integrate it into both the interactive coherence viewer and network plotting workflow.
New Features:
Enhancements: