Updated transect tool #396
Merged
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
Contributor
Reviewer's GuideThis PR refactors the transect tool by replacing the embedded Qt widget plot with a standalone numpy/matplotlib implementation (plot_transect_from_hu), enhances ROI event handling for arrow and wheel interactions, and includes UI clean-ups (whitespace, cursor/pen color tweaks, removal of redundant show calls). Sequence diagram for updated event handling in ROIOptionsControllersequenceDiagram
participant User as actor
participant ROIOptionsController
participant HUD
participant Scroller
User->>ROIOptionsController: Arrow key press (event.type() == 12)
ROIOptionsController->>HUD: setGeometry(viewport.rect())
User->>ROIOptionsController: Mouse wheel event (event.type() == Wheel)
ROIOptionsController->>HUD: setGeometry(viewport.rect())
ROIOptionsController->>Scroller: setValue(scroller.value() +1 or -1)
Class diagram for updated TransectWindow and plot_transect_from_huclassDiagram
class TransectWindow {
+__init__(hu_values=[])
+plot_transect_from_hu(hu_values, pix_spacing=1.0, thresholds_mm=None, is_roi_draw=True, show=True)
}
TransectWindow : QWidget
TransectWindow --> plot_transect_from_hu : uses
Class diagram for ROIOptionsController eventFilter changesclassDiagram
class RoiDrawOptions {
+update_ui(rois, dataset_rtss)
+eventFilter(obj, event)
+set_up(...)
+signal_roi_drawn
+signal_draw_roi_closed
+_hud
+view
+scroller
}
RoiDrawOptions : QtWidgets.QWidget
RoiDrawOptions --> eventFilter : overrides
Class diagram for ButtonBox make_circle_cursor changeclassDiagram
class ButtonBox {
+make_circle_cursor(size, outline, fill, outline_width=3, hotspot_center=True) : QCursor
}
ButtonBox : QtWidgets.QWidget
Class diagram for Canvas pen color change and transect_window updateclassDiagram
class Canvas {
+__init__(pen, slider, rtss)
+t_pen : QPen
+transect_window(point_values)
+canvas
+slice_num
+setPixmap(...)
}
Canvas : QtWidgets.QLabel
Canvas --> TransectWindow : uses
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 there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/View/mainpage/DrawROIWindow/Transect_Window.py:48` </location>
<code_context>
+ "axes": matplotlib.axes.Axes,
+ }
+ """
+ hu_values = np.asarray(hu_values).astype(float)
+ n = hu_values.size
+ if n == 0:
</code_context>
<issue_to_address>
**suggestion:** Consider handling NaN or infinite values in hu_values.
Sanitizing or warning about NaN or infinite values in hu_values before plotting will help prevent unexpected behavior in matplotlib.
</issue_to_address>
### Comment 2
<location> `src/View/mainpage/DrawROIWindow/Transect_Window.py:68` </location>
<code_context>
+ left_mm, right_mm = right_mm, left_mm # ensure left <= right
+
+ # Create figure/axes and step plot
+ fig = plt.figure(num="Transect Graph")
+ ax = fig.add_subplot(111)
+ ax.step(x_mm, hu_values, where="mid")
</code_context>
<issue_to_address>
**suggestion:** Using plt.figure in a QWidget context may cause multiple windows.
Consider using FigureCanvas to embed the matplotlib figure within the QWidget, ensuring a consistent UI and preventing unwanted additional windows.
Suggested implementation:
```python
# Create figure/axes and step plot using FigureCanvas to embed in QWidget
from matplotlib.figure import Figure
from matplotlib.backends.backend_qt5agg import FigureCanvasQTAgg as FigureCanvas
fig = Figure()
ax = fig.add_subplot(111)
ax.step(x_mm, hu_values, where="mid")
# Embed the figure in the QWidget
canvas = FigureCanvas(fig)
# Assuming self is a QWidget and has a layout
self.layout().addWidget(canvas)
```
- If the imports for `Figure` and `FigureCanvasQTAgg` are not already present at the top of the file, add:
```python
from matplotlib.figure import Figure
from matplotlib.backends.backend_qt5agg import FigureCanvasQTAgg as FigureCanvas
```
- Ensure that `self` refers to a QWidget and has a layout set (e.g., `QVBoxLayout`). If not, you may need to initialize a layout for the widget.
- If you want to update the plot multiple times, consider removing previous canvases from the layout before adding a new one.
</issue_to_address>
### Comment 3
<location> `src/View/mainpage/DrawROIWindow/Transect_Window.py:85` </location>
<code_context>
+ roi_x_mm = x_mm[mask]
+ roi_hu = hu_values[mask]
+
+ if show:
+ plt.show(block = False)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Calling plt.show(block=False) may interfere with Qt event loop.
Consider integrating matplotlib with the Qt event loop or using FigureCanvas instead of plt.show(block=False) to avoid event loop conflicts.
</issue_to_address>
### Comment 4
<location> `src/Controller/ROIOptionsController.py:79-83` </location>
<code_context>
self._hud.setGeometry(self.view.viewport().rect())
return False # don't consume; just reacting
- if obj is self.view.viewport() and event.type() == QtCore.QEvent.Wheel:
+ #12 is the number acosiated with the arrow press event
+ #will trigger when the arrow key is pressed
+ if event.type() == 12:
self._hud.setGeometry(self.view.viewport().rect())
- return False # don't consume; just reacting
</code_context>
<issue_to_address>
**suggestion:** Hardcoding event type as 12 reduces code clarity and maintainability.
Use the relevant QtCore.QEvent enum value for arrow key events instead of the number 12 to enhance code readability and maintainability.
```suggestion
# Handle key press events (e.g., arrow keys) using the QtCore.QEvent.KeyPress enum for clarity
if event.type() == QtCore.QEvent.KeyPress:
self._hud.setGeometry(self.view.viewport().rect())
return False
```
</issue_to_address>
### Comment 5
<location> `src/View/mainpage/DrawROIWindow/Canvas.py:282` </location>
<code_context>
self.t_window = TransectWindow(transected_values)
- self.t_window.show()
self.canvas[self.slice_num] = self.transect_pixmap_copy
self.setPixmap(self.canvas[self.slice_num])
</code_context>
<issue_to_address>
**question (bug_risk):** Removing t_window.show() may prevent the window from appearing.
Confirm that TransectWindow is displayed as needed, either through alternative logic or within its own implementation.
</issue_to_address>
### Comment 6
<location> `src/View/mainpage/DrawROIWindow/Transect_Window.py:11` </location>
<code_context>
- self.figure = Figure()
- self.canvas = FigureCanvas(self.figure)
+ def plot_transect_from_hu(self,
+ hu_values,
+ pix_spacing=1.0,
</code_context>
<issue_to_address>
**issue (complexity):** Consider splitting the data preparation and plotting logic in plot_transect_from_hu into separate helper functions for clarity and testability.
```markdown
I’d split `plot_transect_from_hu` into two focused helpers—one for preparing the data (x_mm, thresholds and mask) and one for rendering—so each part is easy to read and test.
```python
# helper #1: data preparation
def _prepare_transect_data(hu_values, pix_spacing=1.0, thresholds_mm=None):
hu = np.asarray(hu_values, float)
if hu.size == 0:
raise ValueError("hu_values is empty.")
x_mm = np.arange(hu.size) * pix_spacing
# default thresholds and sort
if thresholds_mm is None:
thresholds_mm = (x_mm[0], x_mm[-1]) if hu.size > 1 else (x_mm[0], x_mm[0])
left, right = sorted(thresholds_mm)
mask = (x_mm >= left) & (x_mm <= right)
return x_mm, hu, (left, right), mask
```
```python
# helper #2: plotting onto a given Figure/Axes
def _draw_transect(ax, x_mm, hu, thresholds, mask, is_roi_draw):
ax.step(x_mm, hu, where="mid")
ax.set(xlabel="Distance (mm)", ylabel="CT #", title="Transect Graph")
ax.grid(True)
if is_roi_draw:
left, right = thresholds
ax.axvline(left, color="r")
ax.axvline(right, color="r")
return ax
```
And then simplify your main method:
```python
def plot_transect_from_hu(...):
x_mm, hu, thresholds, mask = self._prepare_transect_data(
hu_values, pix_spacing, thresholds_mm
)
fig = Figure()
ax = fig.add_subplot(111)
self._draw_transect(ax, x_mm, hu, thresholds, mask, is_roi_draw)
if show:
plt.show(block=False)
return {
"x_mm": x_mm,
"hu_values": hu,
"thresholds_mm": thresholds,
"roi_x_mm": x_mm[mask] if is_roi_draw else np.array([]),
"roi_hu": hu[mask] if is_roi_draw else np.array([]),
"figure": fig,
"axes": ax,
}
```
This keeps the same behavior but clearly separates data logic from plotting, reduces nesting, and avoids scattering NumPy conversions and threshold logic inside the big method.
</issue_to_address>
### Comment 7
<location> `src/View/mainpage/DrawROIWindow/Transect_Window.py:7` </location>
<code_context>
def __init__(self, hu_values = []):
super().__init__()
self.plot_transect_from_hu(hu_values= hu_values)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
```suggestion
def __init__(self, hu_values=None):
if hu_values is None:
hu_values = []
```
</issue_to_address>
### Comment 8
<location> `src/View/mainpage/DrawROIWindow/Transect_Window.py:58-62` </location>
<code_context>
def plot_transect_from_hu(self,
hu_values,
pix_spacing=1.0,
thresholds_mm=None, # (left_mm, right_mm) along the transect; if None -> (second sample, last sample)
is_roi_draw=True,
show=True,
):
"""
Plot a transect (HU vs distance in mm) from an array of HU values.
Parameters
----------
hu_values : array-like
1D sequence of HU samples along a transect.
pix_spacing : float, optional
Physical spacing per sample (mm per sample). Default 1.0.
thresholds_mm : tuple(float, float) or None, optional
(left_mm, right_mm) bounds for the ROI along the x-axis in millimetres.
If None, defaults to (x[1], x[-1]) when there are >= 2 points, otherwise (x[0], x[0]).
is_roi_draw : bool, optional
If True, draw red threshold lines and compute ROI points.
show : bool, optional
If True, call plt.show() at the end.
Returns
-------
result : dict
{
"x_mm": np.ndarray, # x positions in mm
"hu_values": np.ndarray, # original HU values as np.ndarray
"thresholds_mm": (float, float),
"roi_x_mm": np.ndarray, # x positions inside ROI (empty if is_roi_draw=False)
"roi_hu": np.ndarray, # HU values inside ROI (empty if is_roi_draw=False)
"figure": matplotlib.figure.Figure,
"axes": matplotlib.axes.Axes,
}
"""
hu_values = np.asarray(hu_values).astype(float)
n = hu_values.size
if n == 0:
raise ValueError("hu_values is empty.")
# Build x-axis in millimetres from index * spacing
x_mm = np.arange(n, dtype=float) * float(pix_spacing)
# Choose default thresholds if none provided
if thresholds_mm is None:
if n == 1:
thresholds_mm = (x_mm[0], x_mm[0])
else:
thresholds_mm = (x_mm[0], x_mm[-1])
left_mm, right_mm = thresholds_mm
if left_mm > right_mm:
left_mm, right_mm = right_mm, left_mm # ensure left <= right
# Create figure/axes and step plot
fig = plt.figure(num="Transect Graph")
ax = fig.add_subplot(111)
ax.step(x_mm, hu_values, where="mid")
ax.set_xlabel("Distance (mm)")
ax.set_ylabel("CT # ")
ax.grid(True)
# ROI overlay and data extraction
roi_x_mm = np.array([])
roi_hu = np.array([])
if is_roi_draw:
ax.axvline(left_mm, color="r")
ax.axvline(right_mm, color="r")
mask = (x_mm >= left_mm) & (x_mm <= right_mm)
roi_x_mm = x_mm[mask]
roi_hu = hu_values[mask]
if show:
plt.show(block = False)
return {
"x_mm": x_mm,
"hu_values": hu_values,
"thresholds_mm": (left_mm, right_mm),
"roi_x_mm": roi_x_mm,
"roi_hu": roi_hu,
"figure": fig,
"axes": ax,
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
thresholds_mm = (x_mm[0], x_mm[0]) if n == 1 else (x_mm[0], x_mm[-1])
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
AJB-BigA
requested review from
BurdManMan,
Kahreiru,
Nowhere4Nothing,
Timmy-Codes and
mattbrgn
Kahreiru
approved these changes
Oct 23, 2025
Collaborator
Kahreiru
left a comment
Kahreiru
left a comment
There was a problem hiding this comment.
Looks good need to do something about those print statements still
sjswerdloff
approved these changes
Oct 23, 2025
Collaborator
sjswerdloff
left a comment
sjswerdloff
left a comment
There was a problem hiding this comment.
not sure what the motivation was for the changes, but it looks ok.
Collaborator
Author
The old transect tool was still a dummy version and scroll to change slides was added |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
Updated the transect tool to reflect onko main
Summary by Sourcery
Refactor the transect tool to decouple plotting logic from the Qt UI and enrich it with ROI thresholds, update slice navigation controls, and tweak UI styling
Enhancements: