Skip to content

Port over solution creation and scaling code (#126)#146

Merged
ebrahimebrahim merged 30 commits into
mainfrom
issue_126
Nov 7, 2024
Merged

Port over solution creation and scaling code (#126)#146
ebrahimebrahim merged 30 commits into
mainfrom
issue_126

Conversation

@ltetrel
Copy link
Copy Markdown
Contributor

@ltetrel ltetrel commented Nov 4, 2024

This adds the calc_solution member from Protocol which checks the validity of session targets, setup the simulation scene, beamform the signal, run the simulation, analyze the solution and rescale it.

This currently does not perform any segmentation but create a default water volume (with unform isotropic medium), there is an initial implementation for MRI tissue segmentation that will be re-worked in the future.

This adds the `calc_solution` member from `Protocol` which checks the validity of session targets, setup the simulation scene, beamform the signal, run the simulation, analyze the solution and rescale it.

This currently does not perform any segmentation but create a default water volume (with unform isotropic medium), there is an initial implementation for MRI tissue segmentation that will be re-worked in the future.
@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 4, 2024

Some tests are failing because of json serialization with SolutionAnalysisOptions, if you have suggestions don't hesitate.

@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 4, 2024

I used internally the MNI template (also used in matlab) but git prevents me to commit it (of course). Ideally we would like to add it in the dvc collection when we will work on the MR segmentation.

@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

Some tests are failing because of json serialization with SolutionAnalysisOptions, if you have suggestions don't hesitate.

I'll have a look while I review

Ideally we would like to add it in the dvc collection when we will work on the MR segmentation.

It's in the dvc collection already -- the dvc collection version of "example_subject" contains the full MNI template volume

Adding solution_analysis module to improve import architecture.
@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 5, 2024

Some tests are failing because of json serialization with SolutionAnalysisOptions, if you have suggestions don't hesitate.

I'll have a look while I review

Json serialization with SolutionAnalysisOptions should be fixed now, I forgot to add the associated custom json encoder.

@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 5, 2024

The rest of the tests now are passing. I disabled the simulation to alleviate test runtime, ideally we should enable it but with a reduced parameter set (simulation dt,, time, grid size...).
What do you think @ebrahimebrahim @peterhollender ?

@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

ebrahimebrahim commented Nov 5, 2024

(While reviewing this I am finding it helpful to update the dvc data, so just a heads up that I will push an update to the DVC data to this branch) (never mind I didn't need to do it yet)

Copy link
Copy Markdown
Collaborator

@ebrahimebrahim ebrahimebrahim left a comment

Choose a reason for hiding this comment

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

I gave it a first pass review -- thanks for all this work!

Before we merge also remember:

  • Make all commit messages reference an issue number
  • Would be good to remove any remaining TODO comments at the end, turning them into github issues if they are meant to be addressed in later work.

Comment thread pyproject.toml Outdated
Comment thread src/openlifu/seg/seg_methods/segment_mri.py Outdated
Comment thread src/openlifu/util/geom.py Outdated
Comment thread src/openlifu/sim/sim_setup.py Outdated
Comment thread src/openlifu/sim/sim_setup.py
Comment thread src/openlifu/plan/protocol.py Outdated
Comment thread src/openlifu/plan/protocol.py Outdated
Comment thread src/openlifu/util/units.py Outdated
Comment thread src/openlifu/xdc/transducer.py
Comment thread tests/test_protocol.py Outdated
@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 6, 2024

The rest of the tests now are passing. I disabled the simulation to alleviate test runtime, ideally we should enable it but with a reduced parameter set (simulation dt,, time, grid size...). What do you think @ebrahimebrahim @peterhollender ?

This is currently investigated here: https://github.com/OpenwaterHealth/OpenLIFU-python/pull/146/files/edb303cfe76aca36f667701f9678ad17e06f2bb6#r1830285652

@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 6, 2024

I gave it a first pass review -- thanks for all this work!

Before we merge also remember:

* Make all commit messages reference an issue number

* Would be good to remove any remaining TODO comments at the end, turning them into github issues if they are meant to be addressed in later work.

All commits will be squashed into one commit (rebased to main) as previously. I will keep each commit message for reference.
Yes we should make a pass for all the TODOs, for now I created only #150.

@ltetrel
Copy link
Copy Markdown
Contributor Author

ltetrel commented Nov 6, 2024

@ebrahimebrahim at this stage it would be also important to test the workflow on the Slicer plugin side.

@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

@ebrahimebrahim at this stage it would be also important to test the workflow on the Slicer plugin side.

Yes I am testing that in my review. If you want to test it yourself from a particular commit then I can build a package and share it with you. Or if you were to build it yourself let me know and I can help with what modifications are needed to get it working.

Comment thread src/openlifu/sim/sim_setup.py Outdated
Comment thread src/openlifu/sim/sim_setup.py Outdated
Comment thread src/openlifu/plan/protocol.py Outdated
Copy link
Copy Markdown
Collaborator

@ebrahimebrahim ebrahimebrahim left a comment

Choose a reason for hiding this comment

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

Dropped a couple more comments and replies. Thanks and let me know when I should do another review 😃

Comment thread tests/test_protocol.py Outdated
Comment thread tests/test_protocol.py Outdated
@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

Code looks great!! Just testing it in the slicer app now, feel free to do your squashing if you want.

@ebrahimebrahim
Copy link
Copy Markdown
Collaborator

On second thought let's enable "squash and merge" for this PR only. I don't. So don't squash it yourself please. I want to maintain the history in this branch here and in this PR.

Copy link
Copy Markdown
Collaborator

@ebrahimebrahim ebrahimebrahim left a comment

Choose a reason for hiding this comment

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

It worked perfectly in SlicerOpenLIFU, I will squash and merge shortly!

@ebrahimebrahim ebrahimebrahim linked an issue Nov 7, 2024 that may be closed by this pull request
@ebrahimebrahim ebrahimebrahim merged commit d23aecc into main Nov 7, 2024
@ebrahimebrahim ebrahimebrahim deleted the issue_126 branch March 12, 2025 01:59
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.

Port over solution creation and scaling code

2 participants