-
Notifications
You must be signed in to change notification settings - Fork 7
Port solution analysis functionality from matlab #207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
There are still a few TODOs before this can be merged:
|
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. |
30c1f4a to
c28fa7f
Compare
|
Oops it looks like you're undoing the things I did in your force-push 😄 ... we'll talk soon anyway |
10ba4ab to
90d5843
Compare
Makes type annotation more clear
unit test rescued from the graveyard!
285716f to
6248a8b
Compare
|
@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 |
| 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") |
There was a problem hiding this comment.
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
Adds the solution analysis functionality from MATLAB.