Add conversion layer between napari Shapes and movement regions of interest (RoIs)#927
Draft
Add conversion layer between napari Shapes and movement regions of interest (RoIs)#927
napari Shapes and movement regions of interest (RoIs)#927Conversation
- Replace the single-row HBoxLayout in the layer controls group with a QGridLayout so that the new Save/Load row aligns with the dropdown and Add buttons above it. - Save button is disabled until the current layer has at least one shape (_update_save_button_state, called from _update_table_tooltip). - Both buttons carry descriptive tooltips explaining the GeoJSON workflow. - Update tests: check new buttons exist, verify tooltips, extend the tooltip/state parametrized test to also assert Save button enablement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Implement _load_region_layer: opens a GeoJSON file dialog, calls load_rois and rois_to_napari_shapes, then adds a new region layer named after the file stem. Errors are logged without crashing the widget. - Add imports: Path, QFileDialog, load_rois, rois_to_napari_shapes, logger. - Tests: connection test, cancel-does-nothing test, and parametrized test covering both valid file (layer created with correct name and metadata) and invalid file (error logged, no layer created). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Narrow return annotation from BaseRegionOfInterest to LineOfInterest | PolygonOfInterest, which is what the function always returns. This resolves the mypy list comprehension type mismatch in napari_shapes_to_rois. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #927 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 39 40 +1
Lines 2678 2771 +93
=========================================
+ Hits 2678 2771 +93 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- _save_region_layer: opens a file dialog, converts the current layer's shapes to RoIs via napari_shapes_to_rois, and writes them with save_rois; logs success or error. - _load_region_layer: opens a file dialog, reads RoIs with load_rois, adds them as a new Shapes layer named after the file stem, and selects it in the dropdown; logs success or error. - _update_save_button_state: enables the Save button only when the current layer has at least one shape; called from _update_table_tooltip. - Tests cover button tooltips, save-button enable/disable state, handler connections, cancel (no-op), valid load/save, and error paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The underlying load_rois/save_rois functions already log errors at the source. The widget callbacks only need to surface them in the napari GUI via show_error, consistent with the pattern in loader_widgets.py. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
_ellipse_to_polygon assumed napari stores ellipses as 4 cardinal points (semi-axis endpoints), but napari actually stores the 4 corners of the bounding rectangle. The old code computed semi-axes as centre-to-corner distances (the diagonal), producing polygons ~30% larger than the original ellipse. Fix by deriving semi-axes from the side lengths of the bounding rectangle instead. Also update the test fixture to use bounding box corners and add an area check (π·a·b) to catch size discrepancies. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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
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?
Closes #676.
Users can draw Regions of Interest (RoIs) in the
napariGUI (via #617), but there is no way to convert those shapes tomovementRoI objects for analysis, or to load previously defined RoIs back intonapari.This PR adds the conversion layer that makes both directions possible.
What does this PR do?
naparishapes andmovementRoIs (movement/napari/convert_roi.py).movement.roi.iofunctions that read/write GeoJSON files.boundary_angles.pyexample involving RoIs to refer to drawing regions innapariinstead of defining them programmatically.A sneak-peek of the "Define regions of interest" widget as it looks with these additions:
Conversion functions (
convert_roi.py)The new module lives under
movement/napari/rather thanmovement/roi/. It makes sense to keepnaparian optional dependency that the core RoI module never imports. Better to have thenaparipart of the code import from the core RoI module than the other way around.The module provides four public functions:
roi_to_napari_shape: singlemovementRoI →naparishape (data + shape type)rois_to_napari_shapes: sequence of RoIs → dict of kwargs ready foradd_shapesnapari_shape_to_roi: singlenaparishape →movementRoInapari_shapes_to_rois: entirenapariShapes layer → list of RoIsNote
Question to the NIU reviewer:
Now that we have
movement/napari/convert_roi.py, does it make sense to rename the oldermovement/napari/convert.pytomovement/napari/convert_dataset.pyfor clarity?Known conversion losses
Also documented in docstrings and user guide, but worth calling out here:
"line"and"path"both becomeLineOfInterest;"polygon","rectangle", and"ellipse"all becomePolygonOfInterest. On the way back,LineOfInterestbecomes"path"andPolygonOfInterestbecomes"polygon". I think this is acceptable because the specific shape type is a GUI drawing concern, not an analytical one. What matters is that the geometry is preserved (true except for the following 2 cases) and users can roundtrip without redefining their RoIs by hand.shapelyrepresentation and are approximated as polygons (a logged info message is emitted). Introducing a customEllipseOfInterestclass would depart from theshapely/GeoJSON standard we otherwise follow for RoI geometry, and the polygon approximation is accurate enough for practical analysis purposes.LineOfInterestobjects withloop=Truehave no closed-path equivalent innapari; the closing segment is dropped and a warning is emitted. Loading them as polygons would be more surprising, since a closed line and a filled polygon have different semantics. This is a niche case anyway, as closed lines can only be created via the Python API, not by drawing in the GUI.Save/Load buttons (
regions_widget.py)napari_shapes_to_rois, followed bysave_rois. It is disabled when the layer has no shapes.load_rois, followed byrois_to_napari_shapes, and adds its regions as a new layer named after the file stem, which is then selected in the dropdown.napari.utils.notifications.show_errorto surface errors in the napari GUI. This is consistent withloader_widgets.pyand avoids double-logging, since the underlying I/O functions already log errors at the source.logger.info(also consistent withloader_widgets.py), including the number of regions and file path.References
This is part of the larger #378 effort, and is the missing link that connects #617 with #675.
How has this PR been tested?
tests/test_unit/test_napari_plugin/test_convert_roi.py, including roundtrip tests in both directions.tests/test_unit/test_napari_plugin/test_regions_widget.py, covering: button tooltips, save-button enable/disable state, handler connections, cancel (no-op when dialog is dismissed), valid load/save, and error paths.How to review this PR?
This is mainly addressed to our collaborators at the Keshavarzi Lab, but all are welcome to review! This is how I recommend you start using the new funcitonality.
Quick start: set up a test environment
Clone the repo and check out this branch:
git clone https://github.com/neuroinformatics-unit/movement.git cd movement git fetch origin napari-save-load-regions git checkout napari-save-load-regionsCreate a virtual environment (with conda or uv) and install
movementwith GUI and docs dependencies:conda:
conda create -n movement-review -c conda-forge python=3.13 conda activate movement-review pip install -e ".[dev,docs]"uv:
Build and browse the docs
cd docs make clean htmlOn Windows PowerShell, use
.\make clean htmlinstead.Then open
docs/build/html/index.htmlin your browser and navigate to User Guide > Graphical User Interface > Define regions of interest.Things to try
movement launch, draw some regions, save them to a GeoJSON file, close and reopen the GUI, and load them back.load_roisand use the loaded regions in your Python code.Is this a breaking change?
No.
Does this PR require an update to the documentation?
Checklist: