Skip to content

Conversation

@peterhollender
Copy link
Contributor

Adds the solution analysis functionality from MATLAB.

@peterhollender
Copy link
Contributor Author

There are still a few TODOs before this can be merged:

  1. Remove the previous analysis code, which is largely in openlifu/bf. I can see why we'd want some of these more generic utilities available outside of solution analysis, but I couldn't decide on the best way to structure the code to re-use the code that was already in there. I just stuck all of the methods directly in solution_analysis.py, which made it easy for them to call each other, but that may not be the preferred way
  2. Update unit tests
  3. There is still a small amount of regression between the MATLAB and Python results, but I've traced it down to seemingly different results using MATLAB k-wave's karray.getDistributedSourceSignal k-wave-python's karray.get_distributed_source_signal. I've been picking through them for a while and can't seem to find any differences in my inputs, so this may be a k-wave issue. It's not that different (the signals look the same, they just are numerically a little different), but it is there.

@ebrahimebrahim
Copy link
Collaborator

  1. I just stuck all of the methods directly in solution_analysis.py, which made it easy for them to call each other, but that may not be the preferred way

That makes sense. I agree with putting things where it's easiest to use/organize at first, and then if later we want to expose specific things at a higher level then it's not hard to move them.

@ebrahimebrahim ebrahimebrahim linked an issue Feb 11, 2025 that may be closed by this pull request
@ebrahimebrahim ebrahimebrahim changed the title 141 solution analysis regression Port solution analysis functionality from matlab Feb 18, 2025
@ebrahimebrahim ebrahimebrahim force-pushed the 141-solution-analysis-regression branch from 30c1f4a to c28fa7f Compare February 25, 2025 21:52
@ebrahimebrahim
Copy link
Collaborator

Oops it looks like you're undoing the things I did in your force-push 😄 ... we'll talk soon anyway

@ebrahimebrahim ebrahimebrahim force-pushed the 141-solution-analysis-regression branch 3 times, most recently from 10ba4ab to 90d5843 Compare March 3, 2025 04:39
@ebrahimebrahim ebrahimebrahim force-pushed the 141-solution-analysis-regression branch from 285716f to 6248a8b Compare March 4, 2025 03:22
@ebrahimebrahim ebrahimebrahim marked this pull request as ready for review March 4, 2025 03:25
@ebrahimebrahim
Copy link
Collaborator

@peterhollender Hey Peter, I am ready to merge this. Please let me know verbally if you approve of it before I go ahead.

I had hoped to get unit tests in last week but I didn't get to it. Now to stay within the timeline of our roadmap, I think I should put off the testing. I indicated in this issue what still needs to be tested. We can go back for this once the april 1 milestone is cleared.

For now I added lots of function documentation to make the unit testing process more straightforward later, and also to not lose the fruits of our discussion on this stuff from last week.

@peterhollender
Copy link
Contributor Author

@peterhollender Hey Peter, I am ready to merge this. Please let me know verbally if you approve of it before I go ahead.

I had hoped to get unit tests in last week but I didn't get to it. Now to stay within the timeline of our roadmap, I think I should put off the testing. I indicated in this issue what still needs to be tested. We can go back for this once the april 1 milestone is cleared.

For now I added lots of function documentation to make the unit testing process more straightforward later, and also to not lose the fruits of our discussion on this stuff from last week.

Yep, good for me. I'll start working on the table generation code in a separate PR

@ebrahimebrahim ebrahimebrahim self-requested a review March 4, 2025 15:08
@ebrahimebrahim ebrahimebrahim merged commit 261b101 into main Mar 4, 2025
9 checks passed
ppp_aggregated = solution.simulation_result['p_max'].max(dim="focal_point_index")
# TODO: Ensure this mean is weighted by the number of times each point is focused on, once openlifu supports hitting points different numbers of times
intensity_aggregated = solution.simulation_result['ita'].mean(dim="focal_point_index")
intensity_aggregated = solution.simulation_result['intensity'].mean(dim="focal_point_index")
Copy link
Collaborator

Choose a reason for hiding this comment

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

for other SlicerOpenLIFU developers @sadhana-r @arhowe00 note the rename here from 'ita' to 'intensity' -- it will have to be fixed in SlicerOpenLIFU when the openlifu version is next bumped

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 the rest of the solution analysis code from matlab

3 participants