P/segmethod cleanup #289

peterhollender · 2025-04-24T19:43:53Z

Closes #290
Closes #299

This PR refactors the way SegmentationMethods work and adds a concrete example for transducer tracking in db_dvc. As in #299 , the updated data includes:

an MRI

These can be found in:

db_dvc/subjects/soren/volumes/MRI/MRI.json
db_dvc/subjects/soren/volumes/MRI/MRI_volume.nii

a transducer complete with a registration surface mesh

These can be found in:

db_dvc/transducers/openlifu_2x400/openlifu_2x400.body.stl
db_dvc/transducers/openlifu_2x400/openlifu_2x400.json
db_dvc/transducers/openlifu_2x400/openlifu_2x400.surf.stl

a photoscan and/or photocollection of the same subject who is in the MRI, wearing the same transducer whose registration surface mesh is included in the session

The corresponding photoscans and photocollections were added, and you can see the full list of references/ids in the following files:

db_dvc/subjects/soren/sessions/test session/photoscans/photoscans.json
db_dvc/subjects/soren/sessions/test session/photocollections/photocollections.json

Before merging

Do a merge commit of the db_dvc from main and this branch.

arhowe00

This is looking great! I made some changes to simplify the base class and added type annotations, which have been incredibly helpful. I also added tests for Material and additional tests for SegmentationMethod.

I didn't make changes on more substantive questions I had, so I left those as comments.

When you look at these, I'll see if there are any more tests I can add so this integrates with the app

src/openlifu/seg/seg_methods/uniform.py

arhowe00

Additional question I noticed when testing things out.

src/openlifu/seg/seg_methods/uniform.py

This commit is a wip based on the outcome of Peter's comments regarding the inheritance. If the 3 layer inheritance is preferred, then you will have to subclass this. OpenwaterHealth/OpenLIFU-python#289 (review)

This commit subclasses the `SegmentationMethod` configurer to account for the unique inheritance structure of `SegmentationMethods`. In brief, there are three levels of inheritance, but the last two levels are supposed to be alternative selections. Between the alternative selections, two of them (`UniformWater` and `UniformTissue`) are supposed to be the same as `UniformSegmentation` except that `ref_material` is set as water/tissue and is not intended to be mutable. The subclassing of the definition form takes all these details into account. [See the discussion on GitHub here.](OpenwaterHealth/OpenLIFU-python#289 (comment))

This class was redundant and the functionality was ill-defined. Doing a `git grep` on SlicerOpenLIFU cf446f8aa9793fbb64d1431a5b991d5740fc503e and HEAD revealed no other uses of `get_materials`.

- Simplify and explicitly ignore pyright errors in seg_method tests meant to protect against accidental call errors and misuse.

…t` (#290) Removes string-based shortcuts to ensure type safety. Adds recursive construction of Material instances for clean, nested deserialization.

Consolidated `to_dict` implementation by moving it to the top of the class and simplifying the logic using `__dict__.copy()` with a recursive override for `materials`.

- Also remove comment

Note: `to_dict` should not discard `ref_material` because it is an attribute of the class. However, `from_dict` needs to be overridden to instead account for the overridden constructor which does not use `ref_material` in the construction of the derived objects `UniformTissue` and `UniformWater`.

- Also update associated JSON files to include the `ref_material` for all `SegmentationMethod`s, even if it is not intended to change.

This commit updates db_dvc by running the script scripts/standardize_database.py and adding back `ref_material` for `UniformWater` and `UniformTissue`. Furthermore, the following non-jpg files were removed from the photocollections/ in db_dvc, as photocollections should not contain files of the following kind: ```sh db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_2/texture_1001.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_2/texture_1002.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_2/texturedMesh.mtl db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_2/texturedMesh.obj db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_3/texture_1001.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_3/texture_1002.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_3/texturedMesh.mtl db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_3/texturedMesh.obj db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_4/texture_1001.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_4/texture_1002.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_4/texturedMesh.mtl db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_4/texturedMesh.obj db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_5/texture_1001.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_5/texture_1002.exr db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_5/texturedMesh.mtl db_dvc/subjects/soren/sessions/test session/photocollections/Soren_repeat_5/texturedMesh.obj ```

arhowe00

Looks good. I did test this branch in the extension, and the previous issues with using segmentation methods went away.

This commit subclasses the `SegmentationMethod` configurer to account for the unique inheritance structure of `SegmentationMethods`. In brief, there are three levels of inheritance, but the last two levels are supposed to be alternative selections. Between the alternative selections, two of them (`UniformWater` and `UniformTissue`) are supposed to be the same as `UniformSegmentation` except that `ref_material` is set as water/tissue and is not intended to be mutable. The subclassing of the definition form takes all these details into account. [See the discussion on GitHub here.](OpenwaterHealth/OpenLIFU-python#289 (comment))

arhowe00 self-requested a review April 25, 2025 19:20

arhowe00 force-pushed the p/segmethod_cleanup branch 2 times, most recently from 78cc9cb to 64316b6 Compare April 25, 2025 19:26

This was referenced Apr 28, 2025

Add improvements to protocol config OpenwaterHealth/SlicerOpenLIFU#311

Merged

Add complete solution analysis OpenwaterHealth/SlicerOpenLIFU#284

Merged

arhowe00 force-pushed the p/segmethod_cleanup branch from fc48ddb to 32190b1 Compare May 1, 2025 21:15

ebrahimebrahim mentioned this pull request May 2, 2025

Add a complete example for transducer tracking #299

Closed

arhowe00 requested changes May 5, 2025

View reviewed changes

src/openlifu/seg/seg_methods/uniform.py Outdated Show resolved Hide resolved

src/openlifu/seg/seg_methods/uniform.py Outdated Show resolved Hide resolved

src/openlifu/seg/seg_methods/uniform.py Outdated Show resolved Hide resolved

arhowe00 reviewed May 5, 2025

View reviewed changes

src/openlifu/seg/seg_methods/uniform.py Show resolved Hide resolved

peterhollender and others added 10 commits May 9, 2025 23:39

Refactor SegmentationMethod (#290)

033706f

Re-added dataclass to fix unit tests (#290)

8d88400

Fix ABC, add tests (#290)

2a82e21

Update db_dvc.dvc (#290)

4d8610e

Remove get_materials from Material class (#290)

687e678

This class was redundant and the functionality was ill-defined. Doing a `git grep` on SlicerOpenLIFU cf446f8aa9793fbb64d1431a5b991d5740fc503e and HEAD revealed no other uses of `get_materials`.

Add tests for Material class (#290)

4ef1b6c

Correct seg_method tests (#290)

195bab4

- Simplify and explicitly ignore pyright errors in seg_method tests meant to protect against accidental call errors and misuse.

Simplify and enforce structured input in `SegmentationMethod.from_dic…

5260d5e

…t` (#290) Removes string-based shortcuts to ensure type safety. Adds recursive construction of Material instances for clean, nested deserialization.

Move and simplify SegmentationMethod.to_dict (#290)

12c9e29

Consolidated `to_dict` implementation by moving it to the top of the class and simplifying the logic using `__dict__.copy()` with a recursive override for `materials`.

Improve type safety of _segment (#290)

5ea4cf4

- Also remove comment

arhowe00 force-pushed the p/segmethod_cleanup branch from fd8993a to 21d2ba8 Compare May 10, 2025 03:46

peterhollender and others added 5 commits May 9, 2025 23:47

Accepting ruff to update dvc again (#290)

1a36770

Remove extraneous explicit class attribute in uniform to_dict (#290)

0b7f8e9

Modify SegmentationMethod.from_dict to handle water/tissue (#290)

73035be

- Also update associated JSON files to include the `ref_material` for all `SegmentationMethod`s, even if it is not intended to change.

arhowe00 force-pushed the p/segmethod_cleanup branch from 21d2ba8 to 89f6a64 Compare May 10, 2025 03:47

arhowe00 self-requested a review May 10, 2025 03:47

arhowe00 approved these changes May 10, 2025

View reviewed changes

arhowe00 merged commit 0e09d41 into main May 10, 2025
9 checks passed

arhowe00 mentioned this pull request Jun 10, 2025

Ensure SegmentationMethod Is a proper abstract base class #290

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

P/segmethod cleanup #289

P/segmethod cleanup #289

Uh oh!

peterhollender commented Apr 24, 2025 •

edited by arhowe00

Loading

Uh oh!

arhowe00 left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhowe00 left a comment

Uh oh!

Uh oh!

arhowe00 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

P/segmethod cleanup #289

P/segmethod cleanup #289

Uh oh!

Conversation

peterhollender commented Apr 24, 2025 • edited by arhowe00 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before merging

Uh oh!

arhowe00 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arhowe00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arhowe00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterhollender commented Apr 24, 2025 •

edited by arhowe00

Loading

arhowe00 left a comment •

edited

Loading