Add masking to sliceviewer #39893
Add masking to sliceviewer #39893
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughSummary by CodeRabbit
Walkthrough
Possibly related issues
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/imageinfowidget.py (1)
82-90: Coerce cursor_info points to ints & correct import path
Replacearr = cinfo.array i, j = cinfo.pointwith
arr = cinfo.array i, j = map(int, cinfo.point)to prevent float-indexing errors. Also update your import at line 18 to:
from mantidqt.widgets.sliceviewer.presenters.selector import cursor_info, CursorInfo
🧹 Nitpick comments (17)
qt/python/mantidqt/mantidqt/widgets/regionselector/presenter.py (1)
262-279: Masking stubs: clarify intent or add minimal-safe behaviorThese are currently no-ops. If RegionSelector is not expected to support masking, add docstrings stating they’re intentionally inert. If they may be invoked by shared toolbar actions, at least cancel any in-progress region drawing to avoid stuck UI state.
Proposed minimal-safe implementation:
- def masking(self, active): - pass + def masking(self, active): + """Masking toggle is not supported in RegionSelector. Ensure no region is being drawn.""" + self.cancel_drawing_region() - def rect_masking_clicked(self, active): - pass + def rect_masking_clicked(self, active): + """No-op for interface parity with SliceViewer masking.""" + self.cancel_drawing_region() - def elli_masking_clicked(self, active): - pass + def elli_masking_clicked(self, active): + """No-op for interface parity with SliceViewer masking.""" + self.cancel_drawing_region() - def poly_masking_clicked(self, active): - pass + def poly_masking_clicked(self, active): + """No-op for interface parity with SliceViewer masking.""" + self.cancel_drawing_region() - def export_masking_clicked(self): - pass + def export_masking_clicked(self): + """No-op: RegionSelector does not manage masking export.""" - def apply_masking_clicked(self): - pass + def apply_masking_clicked(self): + """No-op: RegionSelector does not apply masking."""qt/python/mantidqt/mantidqt/widgets/regionselector/test/test_regionselector_presenter.py (1)
28-32: Consistent use of Mock(spec=MatrixWorkspace) improves robustnessReplacing bare Mocks with spec=MatrixWorkspace across tests aligns with RegionSelector’s MATRIX-only constraint and prevents false positives.
Optional: factor a small helper to reduce repetition and improve readability:
def _mw(self): return Mock(spec=MatrixWorkspace)Then use ws=self._mw() throughout.
Also applies to: 53-54, 61-67, 73-79, 81-88, 90-100, 180-188, 232-241, 279-287, 289-294, 297-307, 309-311, 314-321, 326-330, 334-338, 340-353, 356-370, 380-385
qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/selector.py (1)
23-37: Axis order swap is non-obvious — add a brief notedata_extent is constructed as [[ymin, xmin], [ymax, xmax]] and later transform_point([ydata, xdata]) to map to (row, col). It’s correct but surprising.
Add a one-line comment clarifying that we swap (x, y) to map to array (row, col) index space.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/toolbar.py (1)
68-74: Consider grouping the masking toolbar items.The masking tools are currently spread across two separator groups. Consider placing all masking-related actions together in a single group for better UX - either group the shape selection tools (rect/ellipse/polygon) with the main masking toggle, or move them all together:
MantidStandardNavigationTools.SEPARATOR, MantidNavigationTool(ToolItemText.MASKING, "Toggle masking on/off", "mdi.transition-masked", "maskingClicked", False), MantidNavigationTool(ToolItemText.RECT_MASKING, "Select rectangle mask", "mdi.square-outline", "rectMaskingClicked", False), MantidNavigationTool(ToolItemText.ELLI_MASKING, "Select elliptical mask", "mdi.circle-outline", "elliMaskingClicked", False), MantidNavigationTool(ToolItemText.POLY_MASKING, "Select polygon mask", "mdi.triangle-outline", "polyMaskingClicked", False), MantidNavigationTool(ToolItemText.APPLY_MASKING, "Apply drawn mask", "mdi.checkbox-marked-outline", "applyMaskingClicked", None), MantidNavigationTool(ToolItemText.EXPORT_MASKING, "Export drawn mask to table", "mdi.export", "exportMaskingClicked", None), - MantidStandardNavigationTools.SEPARATOR, + MantidStandardNavigationTools.SEPARATOR, MantidNavigationTool(ToolItemText.SAVE, "Save the figure", "mdi.content-save", "save_figure", None),qt/python/mantidqt/mantidqt/widgets/sliceviewer/models/masking.py (5)
125-126: Potential for invalid x_min values with extreme precision.When
x_min == x_max, subtracting10**-8could produce values that might not be properly representable in floating-point arithmetic for very small numbers near zero or at extreme scales.Consider using a relative adjustment instead:
- x_min = x_min - 10**-ALLOWABLE_ERROR if x_min == x_max else x_min + if x_min == x_max: + # Use relative adjustment for better numerical stability + x_min = x_min - max(abs(x_min) * 10**-ALLOWABLE_ERROR, 10**-ALLOWABLE_ERROR)
171-172: Duplicate logic for x_min adjustment.This is identical to line 125-126. Consider extracting this into a helper method to avoid duplication:
def _ensure_distinct_x_values(self, x_min, x_max): """Ensure x_min != x_max by slightly adjusting x_min if necessary.""" if x_min == x_max: return x_min - max(abs(x_min) * 10**-ALLOWABLE_ERROR, 10**-ALLOWABLE_ERROR), x_max return x_min, x_maxThen use it in both places:
- x_min = x_min - 10**-ALLOWABLE_ERROR if x_min == x_max else x_min + x_min, x_max = self._ensure_distinct_x_values(x_min, x_max)
142-143: Consider more descriptive error message.The current error message could be more helpful to users. Consider specifying what kinds of polygons are supported:
- raise RuntimeError("Polygon shapes with more than 1 intersection point are not supported.") + raise RuntimeError("Self-intersecting polygons are not supported. Please ensure your polygon edges do not cross each other (except at vertices).")
161-161: Consider simplifying the complex conditional.The condition checking for infinite slope could be clearer:
- x = (y - line.c) / line.m if (abs(line.m) != inf and abs(line.m) != 0) else line.start[0] + # For vertical lines or near-vertical lines, use the x-coordinate directly + if abs(line.m) == inf or line.m == 0: + x = line.start[0] + else: + x = (y - line.c) / line.m
273-275: Remove commented-out code.Line 274 contains commented-out code that should be removed if not needed:
for row in table_rows: - # if not row.x_min == row.x_max: # the min and max of the ellipse table_ws.addRow([row.spec_list, row.x_min, row.x_max])qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_sliceviewer_imageinfowidget.py (1)
13-19: Import real CursorInfo (with safe fallback) to avoid API drift.Don't re-declare CursorInfo locally; import the production type so changes (e.g., extra fields) don't silently desync tests. Keep a small fallback for older branches if needed.
Apply this diff:
-from collections import namedtuple - -CursorInfo = namedtuple("CursorInfo", ("array", "extent", "point")) +try: + # Prefer the canonical CursorInfo used by presenters + from mantidqt.widgets.sliceviewer.presenters.selector import CursorInfo +except Exception: + # Fallback for older branches + from collections import namedtuple + CursorInfo = namedtuple("CursorInfo", ("array", "extent", "point"))qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_sliceviewer_presenter.py (1)
14-14: Reduce duplication when attaching MatrixWorkspace to mocks.Setting model.ws in many places is repetitive and brittle. Centralize via a small helper/fixture to keep tests concise.
Example helper:
def make_model_with_ws(): m = mock.MagicMock() m.ws = mock.Mock(spec=MatrixWorkspace) return mThen replace repeated snippets with model = make_model_with_ws().
Also applies to: 42-42, 125-129, 154-157, 162-165, 551-552, 561-562, 571-572, 581-582, 591-592, 602-603, 613-614, 630-631, 641-642, 651-652, 661-664
qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/dataviewsubscriber.py (1)
56-79: Consider parameter naming consistency across masking methods.The interface has inconsistent parameter naming - some methods use
activewhile others have no parameters. Consider whether all masking shape selection methods should have consistent signatures for better API clarity.If the
activeparameter is needed for state management, consider making it consistent across all shape selection methods:@abstractmethod def masking(self, active) -> None: pass @abstractmethod def rect_masking_clicked(self, active) -> None: pass @abstractmethod def elli_masking_clicked(self, active) -> None: pass @abstractmethod def poly_masking_clicked(self, active) -> None: pass @abstractmethod -def export_masking_clicked(self) -> None: +def export_masking_clicked(self, active=None) -> None: pass @abstractmethod -def apply_masking_clicked(self) -> None: +def apply_masking_clicked(self, active=None) -> None: passHowever, if
export_masking_clickedandapply_masking_clickedare truly stateless actions (not toggles), the current design is appropriate.qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/base_presenter.py (2)
19-26: Consider documenting the optional model parameter.The model parameter is now optional with a default of None, but this change and its implications aren't documented in the docstring.
Add parameter documentation to clarify the optional model behavior:
def __init__(self, ws, data_view: SliceViewerDataView, model: SliceViewerBaseModel = None): """ Initialize the SliceViewer base presenter. :param ws: The workspace to visualize :param data_view: The data view for displaying the workspace :param model: Optional model for slicing operations. If None, creates a default SliceViewerBaseModel """ self.model = model if model else SliceViewerBaseModel(ws)
130-146: Consider extracting tool state management to reduce duplication.The tool enabling/disabling logic is duplicated between
maskingandregion_selectionmethods. This could lead to maintenance issues if the tool interactions change.Consider extracting common tool state management:
def _set_tool_states(self, zoom_enabled, pan_enabled, region_enabled, masking_enabled): """Helper to manage tool button states.""" tools = [ (ToolItemText.ZOOM, zoom_enabled), (ToolItemText.PAN, pan_enabled), (ToolItemText.REGIONSELECTION, region_enabled), (ToolItemText.MASKING, masking_enabled) ] for tool, enabled in tools: if enabled: self._data_view.enable_tool_button(tool) else: self._data_view.deactivate_and_disable_tool(tool) def masking(self, active) -> None: self._data_view.toggle_masking_options(active) if active: self._set_tool_states(zoom_enabled=False, pan_enabled=False, region_enabled=False, masking_enabled=True) self._data_view.masking = Masking(self._data_view, self.model.ws.name()) self._data_view.masking.new_selector(ToolItemText.RECT_MASKING) # default to rect masking self._data_view.activate_tool(ToolItemText.RECT_MASKING, True) return self._set_tool_states(zoom_enabled=True, pan_enabled=True, region_enabled=True, masking_enabled=True) self._clean_up_masking() self._data_view.check_masking_shape_toolbar_icons(None) self._data_view.canvas.draw_idle()qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/lineplots.py (1)
290-292: Fix unpacking pattern to match the new CursorInfo structure.The code correctly unpacks 4 elements from
cinfo, but the variable name_is not descriptive. Based on the new CursorInfo structure from the selector module, this should be an additional value field. Consider using a more descriptive name or documenting what this field represents.- arr, (xmin, xmax, ymin, ymax), (i, j), _ = cinfo + arr, (xmin, xmax, ymin, ymax), (i, j), value = cinfoqt/python/mantidqt/mantidqt/widgets/sliceviewer/views/dataview.py (1)
74-75: Consider initializing_maskingin__init__instead of using a property setter.The
_maskingattribute is initialized toNonebut has a setter that could be called before the presenter is properly initialized. Consider documenting when this property should be set or add validation in the setter.@masking.setter def masking(self, masking): + """Set the masking handler. Should be called after presenter initialization.""" self._masking = maskingAlso applies to: 209-215
qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py (1)
10-13: Consider making style configurations more flexible.The hardcoded style constants could be made configurable through a settings mechanism or class parameters for better customization.
Consider creating a style configuration class:
class MaskingStyles: def __init__(self, **kwargs): self.shape_style = kwargs.get('shape_style', {"alpha": 0.5, "linewidth": 1.75, "color": "black", "linestyle": "-"}) self.handle_style = kwargs.get('handle_style', {"alpha": 0.5, "markerfacecolor": "gray", "markersize": 4, "markeredgecolor": "gray"}) # ... other styles
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
Framework/Algorithms/src/MaskBins.cpp(1 hunks)Framework/Algorithms/src/MaskBinsFromTable.cpp(2 hunks)docs/source/release/v6.14.0/Workbench/SliceViewer/New_features/000000.rst(1 hunks)qt/python/mantidqt/mantidqt/widgets/regionselector/presenter.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/regionselector/test/test_regionselector_presenter.py(14 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/models/masking.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/base_presenter.py(5 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/imageinfowidget.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/lineplots.py(5 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/presenter.py(3 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/selector.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_base_presenter.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_sliceviewer_imageinfowidget.py(6 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_sliceviewer_presenter.py(16 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/dataview.py(6 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/dataviewsubscriber.py(1 hunks)qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/toolbar.py(3 hunks)
🔇 Additional comments (21)
qt/python/mantidqt/mantidqt/widgets/regionselector/test/test_regionselector_presenter.py (1)
13-13: Import MatrixWorkspace for accurate spec typing — good changeUsing MatrixWorkspace in specs strengthens test fidelity with production type checks.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/toolbar.py (2)
25-30: LGTM! New masking constants follow existing naming pattern.The new ToolItemText constants are properly defined and follow the established naming convention.
43-48: LGTM! Masking signals properly defined.The new Qt signals for masking operations are correctly defined and follow the existing signal naming pattern.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_sliceviewer_imageinfowidget.py (1)
51-53: Mocks look correct with new CursorInfo shape.All mocks set array, extent, point as used by ImageInfoTracker; values and expectations are consistent.
Also applies to: 67-67, 82-82, 99-99, 117-117
Framework/Algorithms/src/MaskBinsFromTable.cpp (1)
78-79: Typo fix + richer debug message LGTM.Framework/Algorithms/src/MaskBins.cpp (1)
30-34: Expanded index-type support is correct; deprecation path handled.Accepting both WorkspaceIndex and SpectrumNum via declareWorkspaceInputProperties keeps BC and enables the new call site.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_base_presenter.py (1)
46-60: LGTM! Test shim correctly implements new abstract methods.The five new masking event handler methods are properly added to the test shim class to maintain compatibility with the abstract base class.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/presenter.py (1)
81-83: LGTM! Masking is properly disabled on initialization.The masking options are correctly disabled until the feature is explicitly activated by the user.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/base_presenter.py (2)
147-151: Good defensive programming with canvas flush.The
flush_events()call before settingmaskingto None ensures all pending matplotlib events are processed, preventing potential race conditions.
152-165: Well-structured masking compatibility check.The
_disable_maskingproperty clearly documents why masking is disabled for certain workspace types and provides a clean extension point for future support.qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/lineplots.py (3)
1-27: LGTM! Import changes align with the new selector module architecture.The changes to consolidate cursor and selector functionality into a dedicated module improve code organization and reduce duplication.
338-347: LGTM! Selector wrapper properly replaces the custom RectangleSelectorMtd class.The new approach using
make_selector_class(RectangleSelector)aligns with the centralized selector architecture and reduces code duplication.
383-391: Ensure consistent handling of CursorInfo structure in rectangle selection.The unpacking pattern correctly handles the new CursorInfo structure, extracting coordinates from
cinfo_release.point. This is consistent with the updated cursor_info implementation.qt/python/mantidqt/mantidqt/widgets/sliceviewer/views/dataview.py (4)
44-46: LGTM! Well-organized constants for masking features.The masking constants are clearly defined and logically grouped into shape options and process options.
154-159: LGTM! Comprehensive signal connections for masking toolbar actions.All masking-related toolbar signals are properly connected to their respective handlers.
546-552: LGTM! Proper propagation of trigger parameter for tool activation/deactivation.The addition of the
triggerparameter allows proper control of signal emission when programmatically changing tool states.
782-790: LGTM! Clean implementation of masking UI helper methods.The helper methods for toggling masking options and updating toolbar icons are well-structured and follow DRY principles.
qt/python/mantidqt/mantidqt/widgets/sliceviewer/presenters/masking.py (4)
1-8: LGTM! Well-organized imports for the new masking module.The imports are properly organized and include all necessary dependencies for the masking functionality.
16-57: LGTM! Well-designed abstract base class for selection masking.The
SelectionMaskingBaseclass properly uses ABC and abstractmethod decorators, providing a solid foundation for concrete selector implementations.
147-166: Good error handling in polygon selection.The polygon selection properly handles RuntimeError with cleanup and logging. This is a good defensive programming practice.
180-232: LGTM! Well-structured masking orchestrator class.The
Maskingclass provides a clean interface for managing multiple selectors and properly handles state transitions between active and inactive selectors.
docs/source/release/v6.14.0/Workbench/SliceViewer/New_features/000000.rst
Outdated
Show resolved
Hide resolved
qt/python/mantidqt/mantidqt/widgets/sliceviewer/models/masking.py
Outdated
Show resolved
Hide resolved
qt/python/mantidqt/mantidqt/widgets/sliceviewer/test/test_sliceviewer_presenter.py
Outdated
Show resolved
Hide resolved
|
@coderabbitai remove all of your comments |
|
@MialLewis I'll resolve all my review comments on this PR. ✅ Actions performedComments resolved. |
sf1919
added
the
ISIS: Core
Description of work
Adds the ability to generate mask table workspaces and apply masks using sliceviewer.
Closes #39908
To test:
Open sliceviewer on the workspace, ensure colorbar is linear scaling.
Click the
Toggle masking on/officon on the toolbar.Draw a selection of masks on the sliceviewer plot using the rectangle, ellipse and polygon tools.
Once drawn, click
export drawn mask to table. Sense check resultant table workspace.Click
Apply drawn mask. Is mask drawn correctly.Stress test the feature. Does the behaviour of the selectors make sense?
Reviewer
Your comments will be used as part of the gatekeeper process. Comment clearly on what you have checked and tested during your review. Provide an audit trail for any changes requested.
As per the review guidelines:
Gatekeeper
As per the gatekeeping guidelines: