plot_coherence_matrix: split image and matrix windows#1493
Merged
yunjunz merged 14 commits intoMay 29, 2026
Merged
Conversation
Contributor
Reviewer's GuideSplits the interactive coherence matrix viewer into two separate matplotlib figures (image and matrix), refactors figure/axes handling accordingly, and updates click handling to keep the two windows synchronized while improving matrix visibility and image markers. Sequence diagram for synchronized image and coherence matrix clickssequenceDiagram
actor User
participant Matplotlib as Matplotlib_canvas
participant Viewer as coherenceMatrixViewer
participant FigImg as fig_img
participant FigMat as fig_mat
User->>FigImg: mouse click
FigImg-->>Matplotlib: button_press_event
Matplotlib-->>Viewer: update_coherence_matrix(event)
alt event.inaxes == ax_img
Viewer->>Viewer: plot_coherence_matrix4pixel(yx)
Viewer->>Viewer: update_image_marker(yx)
Viewer->>FigMat: fig_mat.canvas.draw_idle()
Viewer->>FigMat: fig_mat.canvas.flush_events()
Viewer->>FigImg: fig_img.canvas.draw_idle()
else event.inaxes == ax_mat
Viewer->>Viewer: update_coherence_matrix(event) [pass]
end
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 4 issues, and left some high level feedback:
- Since
update_coherence_matrixdoes nothing forevent.inaxes == self.ax_mat, you can avoid connectingself.fig_matto the callback entirely (or implement a behavior for matrix clicks) to reduce unnecessary event handling. - The logic that sets window titles and calls
tight_layout()for both figures is split betweenplotandplot_init_image/plot_coherence_matrix4pixel; consider centralizing this in one place per figure to avoid duplication and ensure consistent behavior. - In
update_image_marker, removing all artists with marker'^'may unintentionally remove other markers; you could instead track the specific marker (e.g., store the Line2D returned byplot) and update/remove just that one.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `update_coherence_matrix` does nothing for `event.inaxes == self.ax_mat`, you can avoid connecting `self.fig_mat` to the callback entirely (or implement a behavior for matrix clicks) to reduce unnecessary event handling.
- The logic that sets window titles and calls `tight_layout()` for both figures is split between `plot` and `plot_init_image`/`plot_coherence_matrix4pixel`; consider centralizing this in one place per figure to avoid duplication and ensure consistent behavior.
- In `update_image_marker`, removing all artists with marker `'^'` may unintentionally remove other markers; you could instead track the specific marker (e.g., store the Line2D returned by `plot`) and update/remove just that one.
## Individual Comments
### Comment 1
<location path="src/mintpy/plot_coherence_matrix.py" line_range="226-227" />
<code_context>
msg += f'temporal coherence: {tcoh:.2f}'
vprint(msg)
+ self.fig_mat.canvas.manager.set_window_title(self.figname_mat)
+ self.fig_mat.tight_layout()
+
# update figure
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid calling `tight_layout()` on every coherence-matrix update to prevent layout jitter and unnecessary work.
Since `plot_coherence_matrix4pixel` runs on every click, this will call `self.fig_mat.tight_layout()` each time, which is relatively expensive and can cause the matrix and colorbar to shift slightly between interactions. Consider calling `tight_layout()` once after the initial matrix creation (or guarding it with a flag) instead of on every update, unless you truly need to recompute the layout each time.
```suggestion
self.fig_mat.canvas.manager.set_window_title(self.figname_mat)
# call tight_layout only once to avoid jitter and repeated work
if not hasattr(self, "_mat_tight_layout_done"):
self.fig_mat.tight_layout()
self._mat_tight_layout_done = True
```
</issue_to_address>
### Comment 2
<location path="src/mintpy/plot_coherence_matrix.py" line_range="250-254" />
<code_context>
+
+ def update_image_marker(self, yx):
+ """Update the marker point in the image window."""
+ for artist in self.ax_img.get_children():
+ if hasattr(artist, 'get_marker') and artist.get_marker() == '^':
+ artist.remove()
+
+ self.ax_img.plot(yx[1], yx[0], 'r^', markersize=10, markeredgecolor='black')
+ self.fig_img.canvas.draw_idle()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Track the marker artist directly instead of scanning all children and matching by marker symbol.
Scanning all `ax_img` children and relying on `get_marker() == '^'` risks removing unrelated markers that happen to use the same symbol, and it adds per-click overhead. Instead, store a reference to the specific marker when you create it (e.g. `self._marker_artist`), remove it directly if present, and then recreate/update it. This will be more robust and more efficient.
Suggested implementation:
```python
def update_image_marker(self, yx):
"""Update the marker point in the image window."""
# Remove existing marker, if any
marker = getattr(self, "_image_marker", None)
if marker is not None:
marker.remove()
self._image_marker = None
# Create and store new marker
self._image_marker, = self.ax_img.plot(
yx[1],
yx[0],
'r^',
markersize=10,
markeredgecolor='black',
)
self.fig_img.canvas.draw_idle()
```
1. In the class constructor (or an appropriate initialization method), add `self._image_marker = None` so the attribute always exists.
2. If this class is pickled or copied, ensure `_image_marker` is either excluded or correctly reset during those operations, consistent with the rest of your codebase.
</issue_to_address>
### Comment 3
<location path="src/mintpy/plot_coherence_matrix.py" line_range="168" />
<code_context>
- self.cid = self.fig.canvas.mpl_connect('button_press_event', self.update_coherence_matrix)
- if self.disp_fig:
- plt.show()
+ self.fig_img.canvas.manager.set_window_title(self.figname_img)
+ self.fig_img.tight_layout()
return
</code_context>
<issue_to_address>
**suggestion:** Guard against `canvas.manager` being `None` for non-interactive backends.
On some Matplotlib backends `self.fig_img.canvas.manager` can be `None`, which would raise an `AttributeError` here. Please guard this with a check (and do the same for the matrix figure) before calling `set_window_title`.
Suggested implementation:
```python
manager = getattr(getattr(self.fig_img, "canvas", None), "manager", None)
if manager is not None:
manager.set_window_title(self.figname_img)
self.fig_img.tight_layout()
return
```
1. Find the code that sets the window title for the coherence matrix figure (likely something like `self.fig.canvas.manager.set_window_title(self.figname)` or similar).
2. Replace that direct call with the same guarded pattern, e.g.:
```python
manager = getattr(getattr(self.fig, "canvas", None), "manager", None)
if manager is not None:
manager.set_window_title(self.figname)
```
This will ensure both figures behave correctly on non-interactive backends where `canvas.manager` may be `None`.
</issue_to_address>
### Comment 4
<location path="src/mintpy/plot_coherence_matrix.py" line_range="65" />
<code_context>
- self.figname = 'Coherence matrix'
- self.fig_size = None
- self.fig = None
+ self.figname_img = 'Image'
+ self.figsize_img = None
+ self.fig_img = None
</code_context>
<issue_to_address>
**issue (complexity):** Consider consolidating figure-related state and behavior into shared helpers and structures so the two-window plotting logic stays DRY and easier to maintain.
You can keep the new functionality and still cut a fair amount of complexity by consolidating state and factoring out small helpers. Concretely:
### 1. Avoid scattered figure state / duplicated event wiring
Group the two figures into a small structure and iterate over them instead of keeping many parallel attributes and manual connections:
```python
def __init__(self, inps):
# ...
self.figures = {
"img": {"name": "Image", "size": None, "fig": None, "ax": None},
"mat": {"name": "Coherence Matrix", "size": None, "fig": None, "ax": None},
}
self._marker_artist = None
```
Then in `open()` / `plot()`:
```python
def open(self):
# ...
if not self.figures["img"]["size"]:
ds_shape = readfile.read(self.img_file)[0].shape
self.figures["img"]["size"] = pp.auto_figure_size(
ds_shape, disp_cbar=True, scale=0.7
)
if not self.figures["mat"]["size"]:
self.figures["mat"]["size"] = self._auto_matrix_figsize(len(self.date12_list))
# ...
def plot(self):
img_cfg = self.figures["img"]
mat_cfg = self.figures["mat"]
img_cfg["fig"], img_cfg["ax"] = plt.subplots(
num=img_cfg["name"], figsize=img_cfg["size"]
)
mat_cfg["fig"], mat_cfg["ax"] = plt.subplots(
num=mat_cfg["name"], figsize=mat_cfg["size"]
)
self.fig_img, self.ax_img = img_cfg["fig"], img_cfg["ax"]
self.fig_mat, self.ax_mat = mat_cfg["fig"], mat_cfg["ax"]
self._init_fig_layout()
self.plot_init_image()
if all(i is not None for i in self.yx):
self.plot_coherence_matrix4pixel(self.yx)
for fig in (self.fig_img, self.fig_mat):
self._connect_events(fig)
if self.disp_fig:
plt.show()
```
Helper methods:
```python
def _connect_events(self, fig):
fig.canvas.mpl_connect('button_press_event', self.update_coherence_matrix)
def _init_fig_layout(self):
for cfg in (self.figures["img"], self.figures["mat"]):
if cfg["fig"] is None:
continue
cfg["fig"].canvas.manager.set_window_title(cfg["name"])
cfg["fig"].tight_layout()
```
This removes the parallel `figname_*`, `figsize_*`, `fig_*` attributes, and the duplicated `mpl_connect` calls, while keeping behavior identical.
### 2. Extract matrix figure sizing from `open()`
Move the inline size-logic into a dedicated helper:
```python
def _auto_matrix_figsize(self, num_ifg):
if num_ifg <= 50:
return [6, 5]
if num_ifg <= 100:
return [8, 6]
return [10, 8]
```
Now `open()` just calls `_auto_matrix_figsize`, which is easier to find and change.
### 3. Centralize window title / layout calls
Instead of calling `set_window_title` and `tight_layout` in `plot_init_image` and `plot_coherence_matrix4pixel`, let them be pure “data rendering” and keep UI/layout in `_init_fig_layout()` (see above). You can then simplify these methods to only touch axes content:
```python
def plot_init_image(self):
# ... existing data setup ...
self.ax_img = view.plot_slice(self.ax_img, d_img, atr, view_inps)[0]
self.fig_coord = view_inps.fig_coord
def plot_coherence_matrix4pixel(self, yx):
self.ax_mat.cla()
# ... existing matrix computation / imshow / text ...
self.fig_mat.canvas.draw_idle()
self.fig_mat.canvas.flush_events()
```
### 4. Use a single stored marker artist instead of scanning children
Replace the child-scan in `update_image_marker` with a single cached artist:
```python
def __init__(self, inps):
# ...
self._marker_artist = None
def update_image_marker(self, yx):
if self._marker_artist is None:
(self._marker_artist,) = self.ax_img.plot(
yx[1], yx[0], 'r^', markersize=10, markeredgecolor='black'
)
else:
self._marker_artist.set_data([yx[1]], [yx[0]])
self.fig_img.canvas.draw_idle()
```
This makes the intent explicit (“there is exactly one marker”) and avoids per-click iteration over all artists.
### 5. Simplify `update_coherence_matrix` responsibilities
Keep `update_coherence_matrix` as a dispatcher and delegate to smaller helpers:
```python
def update_coherence_matrix(self, event):
if event.inaxes == self.ax_img:
self._handle_image_click(event)
elif event.inaxes == self.ax_mat:
self._handle_matrix_click(event)
def _handle_image_click(self, event):
if self.fig_coord == 'geo':
yx = self.coord.lalo2yx(event.ydata, event.xdata)
else:
yx = [int(event.ydata + 0.5), int(event.xdata + 0.5)]
self.plot_coherence_matrix4pixel(yx)
self.update_image_marker(yx)
def _handle_matrix_click(self, event):
# currently no-op; keep placeholder for future behavior
pass
```
This keeps coordinate transforms, matrix plotting, and marker updates in clearly named helpers, while preserving the behavior you added.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
yunjunz
changed the title
plot_coherence_matrix: split image and matrix windows
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
+ plot init marker: replace view.plot_slice() with update_image_marker() + move update_image_marker() within plot_coherence_matrix4pixel() for call simplicity + show lat/lon info in the terminal whenever it's available
huchangyang
force-pushed
the
feature/coherence-matrix-split-windows-clean
branch
from
d37bda8 to
d7c3bb8
Compare
yunjunz
approved these changes
May 29, 2026
Member
yunjunz
left a comment
yunjunz
left a comment
There was a problem hiding this comment.
Looks all good to me now. Thank you @huchangyang!
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
This PR separates the interactive coherence matrix viewer into two matplotlib windows: one for the reference image/map and one for the coherence matrix.
The previous layout placed both views in a single figure, which made the coherence matrix small for large interferogram networks. With this change, clicking a pixel in the image window still updates the coherence matrix, while the selected pixel marker is refreshed in the image window.
This PR does not change the coherence matrix calculation or add new plotting modes. It only reorganizes the interactive viewer layout to make the matrix easier to inspect.
Reminders
Summary by Sourcery
Split the coherence matrix viewer into separate image and matrix windows to improve matrix visibility while preserving interactive pixel selection.
New Features:
Enhancements: