feat: Allow set units that will be used for load/save data using PartSeg as napari plugin #1228
Conversation
Reviewer's Guide by SourceryThis PR implements a settings editor widget for PartSeg's napari plugin and refactors the plugin's IO handling. The main changes include adding functionality to specify units for data loading/saving and reorganizing the napari plugin-related code structure. Class diagram for PartSegNapariSettings and SettingsEditorclassDiagram
class PartSegNapariSettings {
- PartSegNapariEncoder json_encoder_class
+ Units io_units
+ void set_io_units(Units value)
}
class SettingsEditor {
- PartSegNapariSettings settings
- Widget units_select
+ void units_changed(Units value)
}
PartSegNapariSettings <|-- SettingsEditor
SettingsEditor o-- PartSegNapariSettings: uses
SettingsEditor o-- Widget: contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe pull request introduces modifications to the PartSeg application, primarily focusing on updating hidden imports and import paths related to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Czaki - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| self.units_select.changed.connect(self.units_changed) | ||
| self.append(self.units_select) | ||
|
|
||
| def units_changed(self, value): |
There was a problem hiding this comment.
suggestion: Add error handling for settings.dump() operation
The settings.dump() operation could fail due to I/O errors. Consider wrapping it in a try-except block to handle potential failures gracefully.
Suggested implementation:
def units_changed(self, value):
self.settings.io_units = value
try:
self.settings.dump()
except Exception as e:
import logging
logging.getLogger(__name__).error(f"Failed to save settings: {e}")
# Show error to user if possible
if hasattr(self, "show_error_message"):
self.show_error_message("Failed to save settings")Note: The error handling includes logging and attempts to show a user message if the widget has that capability. You may need to:
- Ensure logging is properly configured in your application
- Implement or modify the show_error_message method if you want to display errors to users
- Consider adding more specific exception types (like IOError) if you know what specific errors might occur
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
package/PartSeg/plugins/napari_widgets/_settings.py (1)
24-31: Consider adding input validation for io_units.The io_units property setter accepts any Units value without validation. Consider adding validation to ensure only supported units are accepted.
@io_units.setter def io_units(self, value: Units): + if not isinstance(value, Units): + raise ValueError(f"Expected Units enum value, got {type(value)}") self.set("io_units", value)package/PartSeg/plugins/napari_io/loader.py (1)
Line range hint
22-38: LGTM: Robust color handling implementationThe function correctly handles different color formats and edge cases. Consider adding more specific type hints for better code maintainability.
Consider updating the type hints to be more specific:
-def adjust_color(color: typing.Union[str, list[int]]) -> typing.Union[str, tuple[float]]: +def adjust_color(color: typing.Union[str, list[int]]) -> typing.Union[str, tuple[float, float, float]]:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
launcher.spec(1 hunks)package/PartSeg/launcher_main.py(1 hunks)package/PartSeg/napari.yaml(3 hunks)package/PartSeg/plugins/napari_io/__init__.py(1 hunks)package/PartSeg/plugins/napari_io/load_image.py(1 hunks)package/PartSeg/plugins/napari_io/load_mask_project.py(1 hunks)package/PartSeg/plugins/napari_io/load_masked_image.py(1 hunks)package/PartSeg/plugins/napari_io/load_roi_project.py(1 hunks)package/PartSeg/plugins/napari_io/loader.py(2 hunks)package/PartSeg/plugins/napari_widgets/__init__.py(2 hunks)package/PartSeg/plugins/napari_widgets/_settings.py(3 hunks)package/tests/test_PartSeg/test_napari_io.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- package/PartSeg/plugins/napari_io/load_mask_project.py
- package/PartSeg/plugins/napari_io/init.py
🔇 Additional comments (13)
package/PartSeg/plugins/napari_widgets/__init__.py (2)
2-2: LGTM: Clean import addition
The import of SettingsEditor is properly placed and follows the module structure, aligning with the PR's objective of introducing settings management functionality.
37-37: LGTM: Proper export declaration
The addition of SettingsEditor to __all__ correctly exposes the new functionality while maintaining alphabetical ordering in the exports list.
package/PartSeg/plugins/napari_io/load_masked_image.py (2)
Line range hint 8-15: LGTM! Well-structured napari reader implementation
The implementation follows napari reader plugin best practices:
- Correctly implements the reader plugin specification
- Efficiently uses generator expression with
next()for path matching - Properly handles the case when no matching reader is found by returning None
- Validates file existence before returning the loader
4-4: Verify the import path change across the codebase
The import path change from PartSegCore.napari_plugins.loader to PartSeg.plugins.napari_io.loader appears to be part of a larger refactoring effort to better organize plugin-related code. Let's verify this change is consistent across the codebase.
✅ Verification successful
Import path change is consistent and properly implemented
The verification shows:
- No instances of the old import path (
PartSegCore.napari_plugins.loader) remain in the codebase - The new import path (
PartSeg.plugins.napari_io.loader) is consistently used across all napari plugin files - The
loader.pyfile exists in the expected new location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old import paths and verify consistency of new imports
# Check for any remaining old imports
echo "Checking for old import paths..."
rg "from PartSegCore.napari_plugins.loader import partseg_loader"
# Verify new import usage
echo "Checking new import paths..."
rg "from PartSeg.plugins.napari_io.loader import partseg_loader"
# Check if loader.py exists in the new location
echo "Verifying loader.py location..."
fd "loader.py" "package/PartSeg/plugins/napari_io/"
Length of output: 895
package/PartSeg/plugins/napari_io/load_image.py (1)
3-3: Verify the import path change impact.
The import path has been updated from PartSegCore.napari_plugins.loader to PartSeg.plugins.napari_io.loader. While this aligns with the refactoring goals, we should verify the new path's existence and backward compatibility.
✅ Verification successful
Import path change is correctly implemented
The verification confirms that:
- The
partseg_loaderfunction exists in the new locationpackage/PartSeg/plugins/napari_io/loader.py - All imports have been updated to use the new path
PartSeg.plugins.napari_io.loader - No references to the old import path
PartSegCore.napari_plugins.loaderremain in the codebase - The function is properly imported and used in multiple napari IO-related modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the new loader module and its exports
ast-grep --pattern 'partseg_loader' package/PartSeg/plugins/napari_io/loader.py
# Check for any remaining references to the old import path
rg "PartSegCore.napari_plugins.loader" -g '!*.md'
Length of output: 232
Script:
#!/bin/bash
# Let's verify the function implementation and its exports
cat package/PartSeg/plugins/napari_io/loader.py
# Check if there are any imports of this function elsewhere
rg "from.*partseg_loader" -g '!*.md'
# Check for any direct usage of partseg_loader
rg "\bpartseg_loader\b" -g '!*.md'
Length of output: 6977
package/PartSeg/plugins/napari_io/load_roi_project.py (1)
4-4: LGTM - Consistent with the import path refactoring.
The import path change is consistent with the changes in load_image.py.
package/PartSeg/plugins/napari_widgets/_settings.py (1)
Line range hint 33-44: LGTM - Return type change is appropriate.
The return type change from PartSettings to PartSegNapariSettings correctly reflects the actual type being returned.
package/PartSeg/plugins/napari_io/loader.py (2)
7-8: LGTM: New imports support unit scaling functionality
The added imports properly support the new feature allowing users to specify units for scale when loading data.
89-90: Verify scaling calculation with different unit combinations
The scaling calculation looks correct, but it's important to verify the behavior with different combinations of input and output units.
✅ Verification successful
Based on the search results, I can verify the scaling calculation implementation. The code shows consistent unit handling across the codebase:
UNIT_SCALEis defined inPartSegCore/universal_const.pyas[10**3, 10**6, 10**9, 10**12], representing scaling factors for different units.- The scaling calculation in
loader.pyusesproject_info.image.normalized_scaling(UNIT_SCALE[settings.io_units.value]), which is consistent with other unit conversions in the codebase. - The test files (particularly
test_measurements.pyandtest_mask_partition.py) demonstrate proper unit conversion and validation of scaling calculations.
Unit scaling implementation is correctly implemented
The scaling calculation in loader.py follows the established pattern used throughout the codebase for unit conversions. The implementation is well-tested and consistent with other unit-handling code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other places where UNIT_SCALE is used to ensure consistent unit handling
rg "UNIT_SCALE" -A 3
# Look for any existing unit conversion tests
fd -e py test_.*unit.*
Length of output: 24476
package/PartSeg/napari.yaml (2)
7-7: LGTM: Consistent plugin path reorganization
The command paths have been consistently updated to reflect the new plugin structure, improving code organization.
Also applies to: 10-10, 13-13, 16-16, 19-19, 22-22, 25-25
80-82: LGTM: Settings Editor widget properly integrated
The Settings Editor widget has been correctly added and registered in both the commands and widgets sections, implementing the PR objective.
Also applies to: 168-169
package/tests/test_PartSeg/test_napari_io.py (1)
11-22: Clean import restructuring for better modularity
The reorganization of imports from PartSegCore.napari_plugins to PartSeg.plugins.napari_io improves the code organization by:
- Separating IO-related functionality into a dedicated module
- Maintaining backward compatibility as test cases remain unchanged
- Following a more intuitive package structure
Let's verify that all napari-related imports are consistently updated across the codebase:
✅ Verification successful
Import restructuring is complete and consistent
The verification shows that:
- No references to the old import path
PartSegCore.napari_pluginsexist in the codebase - The new import path
PartSeg.plugins.napari_iois consistently used across:- The napari plugin configuration (
napari.yaml) - Test files
- Plugin implementation files
- Internal imports within the plugin module
- The napari plugin configuration (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old import path
rg "PartSegCore.napari_plugins"
# Verify the new import path usage
rg "PartSeg.plugins.napari_io"
Length of output: 2344
launcher.spec (1)
93-93: Ensure PyInstaller includes napari IO functionality
The addition of "PartSegCore.napari_io" to hiddenimports is necessary to ensure proper packaging of the napari IO functionality. However, we should verify if both import paths are needed during the transition.
Let's check if both old and new import paths are being used:
✅ Verification successful
The addition of PartSegCore.napari_io to hiddenimports is correct and necessary
The codebase search confirms that the project has transitioned to using the napari_io module extensively, with multiple files under package/PartSeg/plugins/napari_io/ implementing various IO functionalities. The old napari_plugins references are minimal and appear to be for different purposes. The PyInstaller spec file's hiddenimports addition ensures proper packaging of the new napari IO functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both old and new paths are used in the codebase
echo "Checking for old import path usage..."
rg "from PartSegCore.napari_plugins import"
echo "Checking for new import path usage..."
rg "from PartSegCore.napari_io import"
Length of output: 256
Script:
#!/bin/bash
# Let's try a broader search for import patterns and module usage
echo "Checking for napari_plugins module references..."
rg "napari_plugins" -l
echo "Checking for napari_io module references..."
rg "napari_io" -l
echo "Checking PyInstaller spec file content..."
cat launcher.spec
Length of output: 8598
| class SettingsEditor(Container): | ||
| def __init__(self): | ||
| super().__init__() | ||
| self.settings = get_settings() | ||
| self.units_select = create_widget(self.settings.io_units, annotation=Units, label="Units for io") | ||
| self.units_select.changed.connect(self.units_changed) | ||
| self.append(self.units_select) | ||
|
|
||
| def units_changed(self, value): | ||
| self.settings.io_units = value | ||
| self.settings.dump() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for settings persistence.
The settings persistence in units_changed() should handle potential I/O errors when saving settings.
def units_changed(self, value):
self.settings.io_units = value
- self.settings.dump()
+ try:
+ self.settings.dump()
+ except IOError as e:
+ # Consider using a proper logging mechanism or UI notification
+ print(f"Failed to save settings: {e}")Also, consider adding a docstring to explain the widget's purpose and usage.
class SettingsEditor(Container):
+ """Widget for editing PartSeg-specific napari plugin settings.
+
+ This widget provides UI controls for modifying settings such as:
+ - Units used for loading/saving data
+ """
def __init__(self):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class SettingsEditor(Container): | |
| def __init__(self): | |
| super().__init__() | |
| self.settings = get_settings() | |
| self.units_select = create_widget(self.settings.io_units, annotation=Units, label="Units for io") | |
| self.units_select.changed.connect(self.units_changed) | |
| self.append(self.units_select) | |
| def units_changed(self, value): | |
| self.settings.io_units = value | |
| self.settings.dump() | |
| class SettingsEditor(Container): | |
| """Widget for editing PartSeg-specific napari plugin settings. | |
| This widget provides UI controls for modifying settings such as: | |
| - Units used for loading/saving data | |
| """ | |
| def __init__(self): | |
| super().__init__() | |
| self.settings = get_settings() | |
| self.units_select = create_widget(self.settings.io_units, annotation=Units, label="Units for io") | |
| self.units_select.changed.connect(self.units_changed) | |
| self.append(self.units_select) | |
| def units_changed(self, value): | |
| self.settings.io_units = value | |
| try: | |
| self.settings.dump() | |
| except IOError as e: | |
| # Consider using a proper logging mechanism or UI notification | |
| print(f"Failed to save settings: {e}") |
| @@ -33,7 +33,7 @@ def _test_imports(): # pragma: no cover | |||
| raise ImportError("napari_widgets not loaded") | |||
|
|
|||
| if "load_image" not in dir(napari_plugins): | |||
| raise ImportError("napari_plugins not loaded") | |||
| raise ImportError("napari_io not loaded") | |||
There was a problem hiding this comment.
💡 Codebase verification
Import statement needs to be updated for consistency
The import statement from PartSegCore import napari_plugins on line 29 needs to be updated since the codebase now uses napari_io module located in PartSeg.plugins.napari_io. The error message was updated but the import statement was not aligned with this change.
- Update the import in
package/PartSeg/launcher_main.pyto use the new module path - The old import from
PartSegCoreis no longer valid as the functionality has moved toPartSeg.plugins.napari_io
🔗 Analysis chain
Verify import statement consistency
The error message has been updated to reflect the module reorganization, but we should verify that the import statement on line 29 (from PartSegCore import napari_plugins) is also updated accordingly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for remaining references to the old module name
rg "napari_plugins" --type py
# Search for the new module references
rg "napari_io" --type py
Length of output: 2139
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1228 +/- ##
===========================================
+ Coverage 93.12% 93.15% +0.02%
===========================================
Files 210 210
Lines 33178 33217 +39
===========================================
+ Hits 30897 30943 +46
+ Misses 2281 2274 -7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
package/tests/test_PartSeg/test_napari_widgets.py (2)
642-644: Remove unnecessary empty linesThese consecutive empty lines are not needed and should be removed.
644-655: Enhance test coverage and documentationThe test class could be improved in several ways:
- Add docstrings to explain the purpose of each test method
- Enhance
test_createto verify the initial state of the widget- Make
test_change_unitsmore comprehensive by:
- Verifying the initial state
- Testing multiple unit changes
- Testing all available unit options
Consider applying this improvement:
class TestSettingsWidget: + """Test cases for the Settings Editor widget.""" + def test_create(self, qtbot): + """Test that the Settings Editor widget is created correctly.""" w = _settings.SettingsEditor() qtbot.addWidget(w.native) + assert w.units_select.value == _settings.get_settings().io_units def test_change_units(self, qtbot): + """Test that changing units in the widget updates the settings.""" s = _settings.get_settings() s.io_units = Units.µm w = _settings.SettingsEditor() qtbot.addWidget(w.native) + # Verify initial state + assert w.units_select.value == Units.µm + + # Test multiple unit changes w.units_select.value = Units.nm assert s.io_units == Units.nm + + w.units_select.value = Units.mm + assert s.io_units == Units.mm + + # Test all available unit options + for unit in Units: + w.units_select.value = unit + assert s.io_units == unit
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/project_dict.pws(2 hunks)package/PartSeg/common_backend/__init__.py(2 hunks)package/PartSeg/launcher_main.py(1 hunks)package/tests/test_PartSeg/test_napari_widgets.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/project_dict.pws
🚧 Files skipped from review as they are similar to previous changes (1)
- package/PartSeg/launcher_main.py
🔇 Additional comments (3)
package/tests/test_PartSeg/test_napari_widgets.py (1)
48-48: LGTM!
The import of Units from PartSegCore is correctly placed and necessary for the new unit tests.
package/PartSeg/common_backend/__init__.py (2)
5-5: LGTM: Clean import addition
The addition of contextlib import is appropriate for the new error handling approach.
18-21: 🛠️ Refactor suggestion
Consider more specific error handling
Using contextlib.suppress(Exception) silently catches all exceptions, which might hide important errors from users. Consider:
- Catching specific exceptions that you expect (e.g., FileNotFoundError, PermissionError)
- Logging the error for debugging purposes
Here's a suggested improvement:
if path is not None:
- with contextlib.suppress(Exception):
- return _napari_get_settings(path)
+ try:
+ return _napari_get_settings(path)
+ except (FileNotFoundError, PermissionError) as e:
+ import logging
+ logging.debug(f"Failed to load settings from {path}: {e}")
return _napari_get_settings()Let's check what specific exceptions might occur when loading settings:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
package/PartSeg/plugins/napari_widgets/_settings.py (2)
Line range hint
33-43: Add error handling for file operations.The function performs file operations without error handling, which could fail in various scenarios (permissions, disk space, etc.).
Consider wrapping the file operations in try-except blocks:
def get_settings() -> PartSegNapariSettings: global _SETTINGS if _SETTINGS is None: napari_settings = napari_get_settings() if hasattr(napari_settings, "path"): save_path = napari_settings.path else: save_path = os.path.dirname(napari_settings.config_path) - _SETTINGS = PartSegNapariSettings(os.path.join(save_path, "PartSeg_napari_plugins")) - _SETTINGS.load() + try: + _SETTINGS = PartSegNapariSettings(os.path.join(save_path, "PartSeg_napari_plugins")) + _SETTINGS.load() + except (IOError, OSError) as e: + import logging + logging.getLogger(__name__).error(f"Failed to initialize settings: {e}") + # Fallback to in-memory settings + _SETTINGS = PartSegNapariSettings(None) return _SETTINGS
46-53: Add class documentation.The class lacks a docstring explaining its purpose and usage.
Add a descriptive docstring:
class SettingsEditor(Container): + """Widget for editing PartSeg-specific napari plugin settings. + + This widget provides a user interface for modifying settings such as: + - Units used for loading/saving data + """ def __init__(self): super().__init__() self.settings = get_settings()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
package/PartSeg/plugins/napari_widgets/_settings.py(3 hunks)package/tests/test_PartSeg/test_napari_widgets.py(3 hunks)
🔇 Additional comments (3)
package/PartSeg/plugins/napari_widgets/_settings.py (2)
24-31: LGTM! Well-implemented property with proper type hints.
The implementation of the io_units property with getter and setter methods is clean and follows best practices.
55-57: Add error handling for settings persistence.
The settings persistence operation could fail and should be handled gracefully.
package/tests/test_PartSeg/test_napari_widgets.py (1)
644-657: LGTM! Well-structured tests with good coverage.
The test class provides comprehensive coverage of the settings editor functionality, including:
- Widget creation
- Unit value changes
- Settings synchronization
This PR add "Settings Editor" widget to allow edit PartSeg specific settings.
This PR add option to set to which units, the scale should be set when loading to the viewer.
This PR also contains refactor that is mentioned here: #1226 (comment)
Summary by Sourcery
Add a 'Settings Editor' widget to the PartSeg napari plugin to enable editing of specific settings, including setting units for data scaling. Refactor the plugin structure by relocating files to a new 'napari_io' directory for improved organization.
New Features:
Enhancements:
Summary by CodeRabbit
SettingsEditorfor managing unit settings within the PartSeg interface.naparilibrary.