Widget for drawing Regions of Interest (ROIs) as napari Shapes #617
+1,731
−7
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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #617 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 34 35 +1
Lines 2111 2447 +336
==========================================
+ Hits 2111 2447 +336 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
niksirbi
force-pushed
the
napari-roi-widget
branch
2 times, most recently
from
2a48aaa to
3aab669
Compare
|
niksirbi
force-pushed
the
napari-roi-widget
branch
2 times, most recently
from
bc29ead to
b0f665e
Compare
niksirbi
force-pushed
the
napari-roi-widget
branch
from
c0a33d1 to
3b5af63
Compare
niksirbi
changed the title
10 tasks
Member
Author
|
During Friday's Community Call, we discussed enforcing unique region names within a regions layer. This could be added, but I will wait for a PR review and implement it together with other suggestions that will come out of the review. |
Member
Author
|
An edge case I came across today while demoing the feature: if one duplicates an entire regions layer, the new layers is added to the layer selection dropdown, but it isn't automatically selected. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
The dict.update() method returns None, not the updated dictionary. Using the return value directly was causing layer.text to be set to None, which would raise AttributeError when trying to set text.color and text.string afterward. Also use n_shapes variable consistently instead of len(layer.data).
Check if layer exists before accessing its events to avoid AttributeError when layer has been deleted.
- Track connected layers to prevent duplicate signal connections - Disconnect layer name signals when layers are removed - Add closeEvent() to clean up all signal connections on widget close - Use helper methods to manage signal connections consistently
- Ensure names list length matches number of shapes - Auto-assign names to ALL empty/unnamed shapes, not just the last - Rename duplicate ROI-<number> names (likely from paste/duplicate) - Preserve user-assigned names even if duplicated This fixes issues when pasting multiple shapes, duplicating shapes repeatedly, or importing shapes from another layer.
- Add metadata marker (movement_roi_layer) for explicit ROI layers
- Fall back to case-insensitive name matching ("ROI*", "roi*", "Roi*")
- Automatically mark layers with metadata when renamed to ROI pattern
- Once marked, layers remain ROI layers even if renamed
- New layers created via widget are automatically marked
This makes layer identification robust and flexible while respecting
user intent.
- Handle missing name property - Handle empty or short name lists - Handle None or empty string values - Reuse _update_roi_names logic for consistency This ensures existing layers with incomplete naming are properly handled when first linked to the widget.
Use RoisColorManager's default max_layers value instead of explicitly passing the same value.
Clarify that non-numeric ROI suffixes (e.g., 'ROI-center') are intentionally skipped when finding the max number for auto-assignment.
Replace incrementing color index with hash-based selection so the same layer name always gets the same color, even after deletion and recreation. This provides more predictable UX when users recreate layers. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Cover color_all_shapes and color_current_shape methods including the edge case where selected_data contains only invalid indices. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Show different tooltip hints based on widget state: - No ROI layers: prompts user to add a new layer - Empty layer: prompts user to draw shapes - Has shapes: shows usage tips (select, delete, rename) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Assign colors sequentially for "ROIs", "ROIs [1]", "ROIs [2]", etc. to avoid collisions. Custom names use MD5 hash for deterministic assignment across Python sessions. This ensures the first 10 default ROI layers always get distinct colors, while custom names remain consistent across restarts. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Skip frame slider range update when max_frame_idx is negative, which occurs when adding an empty shapes layer (e.g., new ROI layer) without any existing image layers. Add regression test for this edge case. Co-Authored-By: Claude Opus 4.5 <[email protected]>
The _update_roi_names method was defined on RoisTableModel but was being called from RoisWidget._auto_assign_roi_names as self._update_roi_names(), which caused an AttributeError since the method didn't exist on RoisWidget. Fix by moving _update_roi_names to a module-level function that both RoisWidget and RoisTableModel can use. The function doesn't depend on any instance state, so this is the cleanest solution. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove test classes, use plain functions with section headers - Parametrize similar tests to reduce duplication - Combine related assertions into single tests - Remove duplicate tests testing the same functionality Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add _make_roi_name_unique helper that appends [1], [2], etc. to duplicate names, following napari's layer naming convention - Update setData to enforce uniqueness when user renames ROIs - Handle napari's property copying behavior when shapes are added - Change auto-naming format from "ROI-1" to "ROI", "ROI [1]", etc. - Rename _update_roi_names to _fill_empty_roi_names for clarity - Add tests for uniqueness enforcement and edge cases Co-Authored-By: Claude Opus 4.5 <[email protected]>
Replace ROI/ROIs terminology with Region/Regions throughout the napari plugin for better clarity and user-friendliness: - Rename rois_widget.py to regions_widget.py - Rename classes: RoisWidget, RoisTableModel, RoisTableView, RoisStyle, RoisColorManager to their Regions equivalents - Update default layer name from ROIs to Regions - Update metadata key from movement_roi_layer to movement_region_layer - Update UI text and tooltips accordingly - Update all tests and documentation references The movement.roi module retains ROI terminology as it is more of an API/developer concern. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add _on_layer_set_data handler to detect copy-paste operations (napari emits set_data, not data, for copy-paste) - Preserve copied region names instead of overwriting with default - Extract common logic into _sync_names_on_shape_change helper - Add tests for set_data event connection and handler behavior Co-Authored-By: Claude Opus 4.5 <[email protected]>
Explicitly call _update_frame_slider_range() to ensure line 427 is covered when only an empty shapes layer exists. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Tests added to cover: - Line 344: _auto_assign_region_names padding - Line 498: empty selection indexes handling - Line 560: stale index in data() - Lines 571-574: EditRole handling in data() - Line 598: stale index in setData() - Lines 636, 660: layer=None guards in event handlers - Lines 692-693: assign_default_to_new logic - Lines 711-716: _on_layer_deleted cleanup Mark unreachable defensive code with pragma: no cover: - Lines 604-605: padding in setData (napari keeps properties in sync) - Lines 684-685: padding in _sync_names_on_shape_change Co-Authored-By: Claude Opus 4.5 <[email protected]>
sfmig
force-pushed
the
napari-roi-widget
branch
from
3b5af63 to
07ab8c2
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
What is this PR
Why is this PR needed?
See #378.
What does this PR do?
Adds a new
RegionsWidgetto the napari plugin for interactively defining and managing regions of interest. The widget uses Qt's Model/View architecture to display regions drawn in napari Shapes layers.The concept of a "region layer" is key to all of this. A region layer is a Shapes layer that is marked with
movement_region_layerin layer metadata. It can be created by:Once a region layer has been created it remains as such, even through subsequent renames (until deleted).
Users can create one or many region layers, and can draw/edit multiple shapes per region layer. This is useful in situations with many regions, where it would make sense to group them into categories.
New shapes are auto-assigned the default name "Un-named", which can be edited via the table interface.
Key components added:
RegionsWidget: Main widget coordinating the UI, with:RegionsTableModel: Qt model wrapping a napari Shapes layer, exposing region names and shape typesRegionsTableView: Table view with bidirectional selection sync (clicking a row selects the shape in napari and vice versa)RegionsStyle: Style dataclass for consistent region appearance (semi-transparent faces, opaque edges/text)RegionsColorManager: Assigns deterministic colors to region layers based on layer name (sequential for default names, hash-based for custom names)Features:
Note that this PR doesn't cover conversions between
napariShapes andmovementRegionOfInterest (ROI) objects. These have been opened as separate issues and will be tackled in follow-up PRs, see #675 and #676.References
napariShapes layer and our ROI classes #676How has this PR been tested?
Unit tests have been added for
RegionsWidget,RegionsTableModel,RegionsTableView,RegionsStyle, andRegionsColorManager. Coverage forregions_widget.pyis complete, with only 2 unreachable defensive code lines excluded via# pragma: no cover(these handle edge cases that napari's property management currently prevents from occurring in practice).Additional tests were added for
loader_widgets.pyto cover themax_frame_idx < 0case when adding empty shapes layers.Integration tests make more sense once the full workflow exists (load data → draw regions → edit names → save regions), so I'm deferring these for now.
How should this be reviewed
This PR contains quite a lot of lines of code, but most of them are within the 2 new files, and a lot of it is Qt boilerplate.
For me it would be most useful to get feedback on the UI/UX of the new widget. Try using it from the perspective of a user, and break it if you can. The video below shows the envisioned workflow. Keep in mind that I use some shortcuts like Cmd + C and Cmd + V for copy-pasting shapes, and Delete for removing them. All the native napari Shape controls should work as expected.
napari-regions-widget-compressed.mp4
I'm slightly worried that I may have over-engineered this feature, so if you come across things/behaviours that are not needed, feel free to point them out.
Note
I've written the widget tests with the help of Claude, as the commit history shows.
It's my first time experimenting with Claude and I did my best to understand the code it wrote and verify everything. Please review the PR as usual, and do point out anything that doesn't make sense.
Is this a breaking change?
No.
Does this PR require an update to the documentation?
The GUI user guide will be updated once #675 and #676 are also addressed. The new widget is not very useful as is, without the ability to save the regions (alongside their names) to file.
Checklist: