Skip to content

feat(segmentation): add island segmentation effect#51

Merged
Thibault-Pelletier merged 2 commits intomainfrom
island-segmentation
Dec 1, 2025
Merged

feat(segmentation): add island segmentation effect#51
Thibault-Pelletier merged 2 commits intomainfrom
island-segmentation

Conversation

@Jo-Byr
Copy link
Copy Markdown
Collaborator

@Jo-Byr Jo-Byr commented Nov 28, 2025

Add logic and unit test for island segmentation
Add button for island segmentation in medical example

@Thibault-Pelletier Thibault-Pelletier changed the title enh(segmentation): add island segmentation feat(segmentation): add island segmentation Nov 28, 2025
@Thibault-Pelletier Thibault-Pelletier changed the title feat(segmentation): add island segmentation feat(segmentation): add island segmentation effect Nov 28, 2025
Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
Comment thread examples/viewer_lib/ui/segmentation/segment_editor_ui.py Outdated
Comment thread tests/test_segmentation_islands_effect.py Outdated
assert effect.is_active

# Should do nothing
effect.apply(segment_id, None, minimum_size=0, split=False)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please break the unit test in multiple unit tests instead of undo / redo.

As for other parts of the code, unit tests should follow the single responsibility principle and test only one feature at a time.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it better now ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the split.

To make the test better, it's better to make the test name explicit (given / when / then structure).

A better name would be : test_with_0_min_voxel_size_remove_small_islands_does_nothing (please update the different test names)

Please also update the tests to use the specialized API and not a generic apply.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you review the test again ?

Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
@Jo-Byr Jo-Byr force-pushed the island-segmentation branch from 702c5ba to 0095647 Compare November 28, 2025 10:10
Comment thread examples/viewer_lib/logic/segmentation/islands_effect_logic.py Outdated
Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
max_number_of_segments=1,
split=False,
)
elif self._typed_state.data.mode == IslandsSegmentationMode.REMOVE_SMALL_ISLANDS.value:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linked comment with the IslandState dataclass, the typed state mode should be a IslandsSegmentationMode instance directly and not a raw int.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up needing to use encode rather than checking direct equality

Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
Comment thread tests/test_segmentation_islands_effect.py Outdated
Comment thread tests/test_segmentation_islands_effect.py Outdated
Comment thread tests/test_segmentation_islands_effect.py Outdated
Comment thread tests/test_segmentation_islands_effect.py Outdated
Comment thread tests/test_segmentation_islands_effect.py Outdated
Comment thread trame_slicer/segmentation/segmentation.py Outdated
Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
@Jo-Byr Jo-Byr force-pushed the island-segmentation branch from de44948 to 6c7f175 Compare November 28, 2025 15:31
Comment thread tests/examples/test_segment_islands.py Outdated
Comment thread tests/examples/test_segment_islands.py Outdated
Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
Comment thread examples/viewer_lib/ui/segmentation/islands_effect_ui.py Outdated
Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
Comment thread trame_slicer/segmentation/segmentation_effect_islands.py Outdated
@Jo-Byr Jo-Byr force-pushed the island-segmentation branch 2 times, most recently from af13c49 to 5ac8142 Compare December 1, 2025 10:40
Add logic and unit test for island segmentation
Add button for island segmentation in medical example
Bump test data submodule version
@Thibault-Pelletier Thibault-Pelletier merged commit 1da52f8 into main Dec 1, 2025
3 checks passed
@Thibault-Pelletier Thibault-Pelletier deleted the island-segmentation branch December 1, 2025 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants