Port over minimal solution analysis code from matlab (#104) #135
Conversation
This adds minimal solution analysis code so it outputs at least the `mainlobe_pnp_MPa` attribute. The rest has been commented out for future work, additionnal modules/functions are also added to prepare future work.
|
@ltetrel To help with linting issues I recommend installing the pre-commit hooks. Then commits will automatically have some formatting fixes, which deals with most issues you can also run it directly: |
|
@ebrahimebrahim thanks, was just about to do it, |
ebrahimebrahim
left a comment
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Exciting to see this coming together!
I focused on the parts that are involved in getting mainlobe_pnp_MPa and left some broad comments for now
After another round of changes and after we discuss in our meeting soon you can request review again and I will dive into more detail. Perhaps we'll want to merge #127 first as well, as I mention below.
| assert dataclasses_are_equal(Solution.from_files(json_filepath, nc_filepath), example_solution) | ||
|
|
||
|
|
||
| def test_solution_analysis(example_solution: Solution, example_transducer: Transducer): |
There was a problem hiding this comment.
Good to test that we can run the function without error with default options.
Besides this we should also unit-test individual things eventually.
In this PR we should unit-test the functionality that is needed for getting the mainlobe_pnp_MPa:
calc_dist_from_focusoffset_gridmask_focusrescale_coordsget_ndgrid_from_arr
Having these unit tests will be more important than moving on to #105 to get the rest of the solution analysis code. Because these are components that will directly be involved in determining how to scale the pulse to be requested from the hardware.
There was a problem hiding this comment.
Yep makes sense, do we want to have tests against a reference data, or only the run (like for analyze)?
There was a problem hiding this comment.
I think not necessarily reference data but just tests on small toy examples to verify that the individual functions do what they are intended to do.
src/openlifu/plan/solution.py
Outdated
| foc, | ||
| options.mainlobe_radius, | ||
| mask_op=MaskOp.LESS_EQUAL, | ||
| units="m", |
There was a problem hiding this comment.
These units "m" are the units of the options.mainlobe_radius, right? If so I think that the units really should be specified in that options object, or at least there should be a docstring under the mainlobe_radius attribute of the SolutionOptions class indicating that it must be specified in meters
There was a problem hiding this comment.
From my understanding, aspect_ratio does not have units, it is just a scalar to scale and take into account voxel anisotropy. the units here are used to defined to what units all the spatial dimension should be rescaled to.
@peterhollender you coonfirm ?
Fix circular import issue Co-authored-by: Ebrahim Ebrahim <ebrahim.ebrahim@kitware.com>
distance units now available in SolutionOptions
Removed `calc_dist_from_focus`, the algorithm is now in `mask_focus` pre-commit hook
|
@ebrahimebrahim as suggested, there may be some regression in the solution analysis When you agree with the current status, I will squash everything so we have only one commit. |
|
Sounds good and thank you for all this work -- will review soon (later today or early Mon) and let you know |
ebrahimebrahim
left a comment
ebrahimebrahim
left a comment
There was a problem hiding this comment.
I tried it out, everything looks good as far as I can tell. Thank you for all the work on this and the unit tests. As we discussed you can go ahead and open an issue to track the fact that we do not yet know for sure that analyze is producing the correct output. I think the same of get_beamwidth, right? It is not unit tested and we don't know that it is working for sure, so you can mention that in the issue as well.
|
OOPS. |
This adds minimal solution analysis code so it outputs at least the
mainlobe_pnp_MPaattribute. The rest has been commented out for future work, additionnal modules/functions are also added to prepare future work.Please check the #TODO comments in the code for technical discussions.