feat: Add metadata viewer as napari widget #1195
Conversation
Reviewer's Guide by SourceryThis pull request adds a new feature to display metadata for layers in napari as a widget. The implementation includes a new LayerMetadata class, updates to existing files to support this feature, and corresponding test cases. File-Level Changes
Tips
|
WalkthroughThe changes involve several enhancements to the PartSeg package, including the introduction of a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Napari
participant LayerMetadata
User->>Napari: Open viewer
Napari->>LayerMetadata: Initialize LayerMetadata
LayerMetadata->>LayerMetadata: Create layer selector
LayerMetadata->>LayerMetadata: Populate metadata
User->>LayerMetadata: Select image layer
LayerMetadata->>LayerMetadata: Update displayed metadata
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (6)
Additional comments not posted (15)
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 - here's some feedback:
Overall Comments:
- There's a typo in the filename 'metadata_wiewer.py'. It should be 'metadata_viewer.py'.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| @@ -0,0 +1,29 @@ | |||
| import napari | |||
There was a problem hiding this comment.
issue (typo): Fix typo in filename: 'metadata_wiewer.py' should be 'metadata_viewer.py'
Ensure consistency in naming across the project. This typo should be corrected in all references to this file, including import statements and documentation.
| def reset_choices(self): | ||
| self.layer_selector.reset_choices() | ||
|
|
||
| def update_metadata(self): |
There was a problem hiding this comment.
suggestion: Consider adding error handling and user feedback in update_metadata method
The method could benefit from handling cases where the selected layer has no metadata. Consider adding a check and displaying a message to the user if no metadata is available.
def update_metadata(self):
if self.layer_selector.value is None:
return
try:
metadata = self.layer_selector.value.metadata
if not metadata:
raise ValueError("No metadata available")
# Process metadata here
except (AttributeError, ValueError) as e:
# Handle the error (e.g., display a message to the user)
print(f"Error updating metadata: {str(e)}")
|
|
||
| self.setLayout(layout) | ||
| self.update_metadata() | ||
| self.layer_selector.changed.connect(self.update_metadata) |
There was a problem hiding this comment.
suggestion (performance): Evaluate performance impact of updating metadata on every layer change
For large metadata sets, updating on every layer change could potentially cause UI lag. Consider implementing a debounce mechanism or updating only when the metadata view is active.
from functools import debounce
# ... (in the __init__ method)
self.layer_selector.changed.connect(self.debounced_update_metadata)
@debounce(0.3)
def debounced_update_metadata(self):
self.update_metadata()
There was a problem hiding this comment.
There is no debounce in funtols
| class TestLayerMetadata: | ||
| def test_init(self, make_napari_viewer, qtbot): | ||
| viewer = make_napari_viewer() | ||
| widget = LayerMetadata(viewer) | ||
| qtbot.addWidget(widget) | ||
| assert widget._dict_viewer._data == {} |
There was a problem hiding this comment.
suggestion (testing): Test case for empty metadata initialization
This test case checks the initialization of LayerMetadata with an empty viewer. Consider adding assertions to verify the initial state of other widget components, such as the layer_selector.
| class TestLayerMetadata: | |
| def test_init(self, make_napari_viewer, qtbot): | |
| viewer = make_napari_viewer() | |
| widget = LayerMetadata(viewer) | |
| qtbot.addWidget(widget) | |
| assert widget._dict_viewer._data == {} | |
| class TestLayerMetadata: | |
| def test_init(self, make_napari_viewer, qtbot): | |
| viewer = make_napari_viewer() | |
| widget = LayerMetadata(viewer) | |
| qtbot.addWidget(widget) | |
| assert widget._dict_viewer._data == {} | |
| assert widget.layer_selector.count() == 0 | |
| assert widget.layer_selector.currentText() == "" | |
| assert not widget.metadata_table.rowCount() |
| def test_init_with_layer(self, make_napari_viewer, qtbot): | ||
| viewer = make_napari_viewer() | ||
| viewer.add_image( | ||
| np.ones((10, 10)), | ||
| contrast_limits=[0, 1], | ||
| metadata={"foo": "bar"}, | ||
| ) | ||
| widget = LayerMetadata(viewer) | ||
| qtbot.addWidget(widget) | ||
| assert widget._dict_viewer._data == {"foo": "bar"} |
There was a problem hiding this comment.
suggestion (testing): Test case for initialization with a layer
This test verifies the initialization of LayerMetadata with a viewer containing a layer with metadata. Consider adding assertions to check if the layer_selector is properly populated and if the correct layer is selected.
def test_init_with_layer(self, make_napari_viewer, qtbot):
viewer = make_napari_viewer()
layer = viewer.add_image(
np.ones((10, 10)),
contrast_limits=[0, 1],
metadata={"foo": "bar"},
)
widget = LayerMetadata(viewer)
qtbot.addWidget(widget)
assert widget._dict_viewer._data == {"foo": "bar"}
assert widget.layer_selector.count() == 1
assert widget.layer_selector.currentText() == layer.name
| ) | ||
| widget = LayerMetadata(viewer) | ||
| qtbot.addWidget(widget) | ||
| assert widget._dict_viewer._data == {"foo": "bar"} |
There was a problem hiding this comment.
suggestion (testing): Test case for adding a layer after initialization
This test checks the behavior when a layer is added after the widget is initialized. Consider adding a test to verify the behavior when multiple layers are added or when layers are removed.
def test_multiple_layers(self, make_napari_viewer, qtbot):
viewer = make_napari_viewer()
widget = LayerMetadata(viewer)
qtbot.addWidget(widget)
viewer.add_image(np.ones((10, 10)), metadata={"foo": "bar"})
viewer.add_image(np.zeros((5, 5)), metadata={"baz": "qux"})
widget.reset_choices()
assert "foo" in widget._dict_viewer._data
assert "baz" in widget._dict_viewer._data
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (2)
package/PartSeg/plugins/napari_widgets/metadata_wiewer.py (1)
26-29: Consider using a guard clause to simplify theupdate_metadatamethod.The
update_metadatamethod can be slightly refactored to use a guard clause, which can improve readability and reduce nesting.Apply this diff to refactor the method:
def update_metadata(self): - if self.layer_selector.value is None: - return - self._dict_viewer.set_data(self.layer_selector.value.metadata) + if self.layer_selector.value is not None: + self._dict_viewer.set_data(self.layer_selector.value.metadata)package/tests/test_PartSeg/test_napari_widgets.py (1)
633-636: Please clarify the purpose of this test.The
test_enumfunction checks if the string representation ofCompareType.lower_thresholdandFlowType.dark_centerenums contains a space character. It would be helpful to understand the significance of this check and why the presence of a space character is important. Consider adding comments to explain the reasoning behind this test.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- package/PartSeg/common_gui/dict_viewer.py (1 hunks)
- package/PartSeg/napari.yaml (2 hunks)
- package/PartSeg/plugins/napari_widgets/init.py (2 hunks)
- package/PartSeg/plugins/napari_widgets/metadata_wiewer.py (1 hunks)
- package/PartSegCore/napari_plugins/loader.py (1 hunks)
- package/tests/test_PartSeg/test_napari_widgets.py (2 hunks)
Additional comments not posted (10)
package/PartSeg/plugins/napari_widgets/metadata_wiewer.py (1)
9-29: LGTM!The
LayerMetadataclass is well-structured and follows good practices:
- The class is properly initialized with a Napari viewer.
- The UI is set up using a
QVBoxLayoutwith a layer selector and a dictionary viewer.- The
reset_choicesmethod provides a way to reset the layer selector choices.- The
update_metadatamethod updates the dictionary viewer with the metadata of the selected layer.package/PartSeg/plugins/napari_widgets/__init__.py (2)
15-15: LGTM!The new import statement for
LayerMetadatais consistent with the existing imports and follows the correct syntax and naming conventions. The AI-generated summary confirms that this change is intentional and enhances the module's functionality by making theLayerMetadatawidget available for use.
31-31: LGTM!The addition of
LayerMetadatato the__all__list is consistent with the new import statement added earlier. The AI-generated summary confirms that this change is intentional and reflects the addition of theLayerMetadatawidget to the list of exported entities.package/PartSeg/common_gui/dict_viewer.py (1)
19-19: LGTM!The change to use a dedicated method
set_datafor setting the input data is a good practice for encapsulation. It allows for better control over the data assignment logic and provides a centralized place for handling the input data, including the case when it isNone.The method also ensures that the widget state is consistent with the input data by clearing the tree and filling it with the new data.
Overall, this change enhances the structure of the code and does not introduce any apparent issues.
package/PartSegCore/napari_plugins/loader.py (1)
29-29: LGTM!The change looks good. Adding the image metadata to the layer configuration can provide useful contextual information for further processing or display.
package/PartSeg/napari.yaml (2)
77-79: LGTM!The new contribution entry for the "View Layer Metadata" feature is properly defined, and the
python_namefield correctly references theLayerMetadatawidget in thePartSeg.plugins.napari_widgetsmodule.
163-164: LGTM!The new command entry in the
widgetssection for the "View Layer Metadata" feature is properly defined, and thecommandfield correctly references the corresponding contribution entry with the idPartSeg.Metadata.package/tests/test_PartSeg/test_napari_widgets.py (3)
603-608: LGTM!The
test_initmethod correctly tests the initialization ofLayerMetadatawidget when no layers are present in the viewer. The assertion verifies the expected behavior.
609-619: LGTM!The
test_init_with_layermethod correctly tests the initialization ofLayerMetadatawidget when an image layer with metadata is present in the viewer. The assertion verifies that the widget's internal data dictionary reflects the layer's metadata.
620-632: LGTM!The
test_add_layer_post_initmethod correctly tests the behavior ofLayerMetadatawidget when a layer is added after the widget initialization. The assertions verify that the widget's internal data dictionary is initially empty and updates correctly to reflect the metadata of the newly added image layer after callingwidget.reset_choices().
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
package/PartSeg/plugins/napari_widgets/metadata_viewer.py (1)
9-29: LGTM! TheLayerMetadataclass is well-designed and implemented.The class provides a useful feature to display layer metadata in the napari viewer. It integrates well with the existing napari viewer and layer system, and the use of
magicguiandqtpywidgets ensures a consistent UI experience.To further improve the code quality, consider adding the following:
- Type hints for the
layer_selectorand_dict_viewerattributes:from magicgui.widgets import Widget from PartSeg.common_gui.dict_viewer import DictViewer class LayerMetadata(QWidget): def __init__(self, viewer: napari.Viewer): ... self.layer_selector: Widget = create_widget(annotation=NapariImage, label="Layer", options={}) self._dict_viewer: DictViewer = DictViewer() ...
- Docstrings for the class and its methods:
class LayerMetadata(QWidget): """A widget to display the metadata of a selected layer in a napari viewer.""" def __init__(self, viewer: napari.Viewer): """Initialize the LayerMetadata widget. Parameters ---------- viewer : napari.Viewer The napari viewer instance. """ ... def reset_choices(self): """Reset the choices in the layer selector widget.""" ... def update_metadata(self): """Update the displayed metadata based on the selected layer.""" ...
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- package/PartSeg/plugins/napari_widgets/init.py (2 hunks)
- package/PartSeg/plugins/napari_widgets/metadata_viewer.py (1 hunks)
- package/tests/test_PartSeg/test_napari_widgets.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- package/PartSeg/plugins/napari_widgets/init.py
- package/tests/test_PartSeg/test_napari_widgets.py
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1195 +/- ##
===========================================
+ Coverage 93.07% 93.08% +0.01%
===========================================
Files 209 210 +1
Lines 32927 32974 +47
===========================================
+ Hits 30646 30694 +48
+ Misses 2281 2280 -1 ☔ View full report in Codecov by Sentry. |
|
@coderabbitai full review |
Actions performedFull review triggered. |
Summary by Sourcery
Add a new LayerMetadata widget to the napari viewer for displaying layer metadata, update the DictViewer class to use a set_data method, and include tests for the new widget.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Release Notes
New Features
LayerMetadatawidget for selecting layers and displaying their metadata.Bug Fixes
Tests
LayerMetadatawidget to ensure proper functionality during initialization and layer updates.