Skip to content

Add heterogenous param extraction #259

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jun 9, 2025
Merged

Add heterogenous param extraction #259

merged 3 commits into from
Jun 9, 2025

Conversation

@peterhollender
Copy link
Contributor

@peterhollender peterhollender commented Mar 26, 2025

Closes #258

@peterhollender peterhollender linked an issue Mar 26, 2025 that may be closed by this pull request
k-Wave simulation ignore heterogenous params #258
Closed
@peterhollender
Copy link
Contributor Author

peterhollender commented May 27, 2025

@ebrahimebrahim I'm not sure why CI is failing on macOS with an overflow error - does that CI result mean something to you?

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented May 27, 2025

That is interesting...

@arhowe00 can you try this on your system? Just checkout the branch and run pytest -k test_sim -- do you get an overflow error?

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented May 27, 2025

@peterhollender We have a minimal test here that just checks that the simulation runs at all, to help catch issues we've had in the past with a dependency update breaking kwave on one platform or python version:

https://github.com/OpenwaterHealth/OpenLIFU-python/blob/main/tests/test_sim.py

When you look at that test do you see anything stand out as being unreasonable parameters that could cause overflow?

@arhowe00
Copy link
Contributor

arhowe00 commented May 29, 2025 •
edited
Loading

@arhowe00 can you try this on your system? Just checkout the branch and run pytest -k test_sim -- do you get an overflow error?

I also got a failure here. However, I'm using Python 3.12. It threw within the logger, I can investigate and debug tomorrow:

self = <LogRecord: root, 20, /Users/andrew.howe/projects/ow/OpenLIFU-python/env-3.12.2/lib/python3.12/site-packages/kwave/kWaveSimulation_helper/set_sound_speed_ref.py, 89, "  reference sound speed: ">

    def getMessage(self):
        """
        Return the message for this LogRecord.

        Return the message for this LogRecord after merging any user-supplied
        arguments with the message.
        """
        msg = str(self.msg)
        if self.args:
>           msg = msg % self.args
E           TypeError: not all arguments converted during string formatting

msg        = '  reference sound speed: '
self       = <LogRecord: root, 20, /Users/andrew.howe/projects/ow/OpenLIFU-python/env-3.12.2/lib/python3.12/site-packages/kwave/kWaveSimulation_helper/set_sound_speed_ref.py, 89, "  reference sound speed: ">

../../../.pyenv/versions/3.12.2/lib/python3.12/logging/__init__.py:392: TypeError
...
FAILED tests/test_sim.py::test_run_simulation_runs - TypeError: not all arguments converted during string formatting

@arhowe00
Copy link
Contributor

arhowe00 commented May 29, 2025 •
edited
Loading

It's probably best first to debug the error in the CI, as my installation has a few workarounds.

So apparently squaring output['p_min'].reshape(sz, order='F') is causing overflow. float64 overflows at 1e308. For reference, this happens here:

>       intensity = xa.DataArray(1e-4*output['p_min'].reshape(sz, order='F')**2/(2*Z),
                             coords=params.coords,
                             name='I',
                             attrs={'units':'W/cm^2', 'long_name':'Intensity'})

and some of these values for p_min look very large, like -3.4028235e+38 will square to 1e77, I assume there may be very large values in an array.

output     = {'Nt': array(3, dtype=uint64), 'Nx': array(81, dtype=uint64), 'Ny': array(81, dtype=uint64), 'Nz': array(81, dtype=uint64), ...}
p_max      = <xarray.DataArray 'p_max' (lat: 21, ele: 21, ax: 13)> Size: 23kB
array([[[-3.4028235e+38, -3.4028235e+38, -3.4028235e+...    (ax) float64 104B -2.0 -1.0 0.0 1.0 2.0 ... 6.0 7.0 8.0 9.0 10.0
Attributes:
    units:      Pa
    long_name:  PPP
p_min      = <xarray.DataArray 'p_min' (lat: 21, ele: 21, ax: 13)> Size: 23kB
array([[[-3.4028235e+38, -3.4028235e+38, -3.4028235e+...    (ax) float64 104B -2.0 -1.0 0.0 1.0 2.0 ... 6.0 7.0 8.0 9.0 10.0
Attributes:
    units:      Pa
    long_name:  PNP

@peterhollender
Copy link
Contributor Author

peterhollender commented May 29, 2025

It's probably best first to debug the error in the CI, as my installation has a few workarounds.

So apparently squaring output['p_min'].reshape(sz, order='F') is causing overflow. float64 overflows at 1e308. For reference, this happens here:

>       intensity = xa.DataArray(1e-4*output['p_min'].reshape(sz, order='F')**2/(2*Z),
                             coords=params.coords,
                             name='I',
                             attrs={'units':'W/cm^2', 'long_name':'Intensity'})

and some of these values for p_min look very large, like -3.4028235e+38 will square to 1e77, I assume there may be very large values in an array.

output     = {'Nt': array(3, dtype=uint64), 'Nx': array(81, dtype=uint64), 'Ny': array(81, dtype=uint64), 'Nz': array(81, dtype=uint64), ...}
p_max      = <xarray.DataArray 'p_max' (lat: 21, ele: 21, ax: 13)> Size: 23kB
array([[[-3.4028235e+38, -3.4028235e+38, -3.4028235e+...    (ax) float64 104B -2.0 -1.0 0.0 1.0 2.0 ... 6.0 7.0 8.0 9.0 10.0
Attributes:
    units:      Pa
    long_name:  PPP
p_min      = <xarray.DataArray 'p_min' (lat: 21, ele: 21, ax: 13)> Size: 23kB
array([[[-3.4028235e+38, -3.4028235e+38, -3.4028235e+...    (ax) float64 104B -2.0 -1.0 0.0 1.0 2.0 ... 6.0 7.0 8.0 9.0 10.0
Attributes:
    units:      Pa
    long_name:  PNP

Wow, this is not at all what I get on my machine...
p_min.min() is -3194.65 and p_min.max() is 0.0, so intensity.max() is just .00034019.

I got the values by debugging test_sim.test_run_simulation_runs() and putting a breakpoint in kwave_if.py after the intensity calculation.

@peterhollender
Copy link
Contributor Author

peterhollender commented May 29, 2025

@peterhollender We have a minimal test here that just checks that the simulation runs at all, to help catch issues we've had in the past with a dependency update breaking kwave on one platform or python version:

https://github.com/OpenwaterHealth/OpenLIFU-python/blob/main/tests/test_sim.py

When you look at that test do you see anything stand out as being unreasonable parameters that could cause overflow?

Values look fine on Windows. It's certainly a reductive simulation which could be like... falling apart (if you undersample sims, sometimes it goes haywire at boundaries), but I don't know why we'd see that only on mac, while windows remains well-behaved

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented May 30, 2025

I figured out the logger bug that we were getting when running pytest -k test_sim: #328

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented May 30, 2025 •
edited
Loading

Peter we've looked at the macos overflow issue and narrowed it down to the kwave binary doing different things on different systems.

In all cases, the input file that gets delivered into the kwave binary looks like this (this is a print so arrays can get elided, but hopefully enough of the idea is there to understand):

input to kwave 
Nt [[[3]]]
Nx [[[81]]]
Ny [[[81]]]
Nz [[[81]]]
absorbing_flag [[[1]]]
alpha_coeff [[[0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  ...
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]]

 [[0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  ...
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]]

 [[0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  ...
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]]

 ...

 [[0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  ...
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]]

 [[0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  ...
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]]

 [[0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  ...
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]
  [0. 0. 0. ... 0. 0. 0.]]]
alpha_power [[[0.9]]]
axisymmetric_flag [[[0]]]
c0 [[[1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  ...
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]]

 [[1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  ...
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]]

 [[1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  ...
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]]

 ...

 [[1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  ...
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]]

 [[1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  ...
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]]

 [[1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  ...
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]
  [1500. 1500. 1500. ... 1500. 1500. 1500.]]]
c_ref [[[1500.]]]
dt [[[2.e-07]]]
dx [[[0.001]]]
dy [[[0.001]]]
dz [[[0.001]]]
elastic_flag [[[0]]]
nonlinear_flag [[[0]]]
nonuniform_grid_flag [[[0]]]
p0_source_flag [[[0]]]
p_source_flag [[[63]]]
p_source_index [[[238657 238658 238659 238660 238661 238662 238663 238664 238665 238666
   238667 238668 238669 238670 238671 238672 238673 238674 238675 238676
   238677 238738 238739 238740 238741 238742 238743 238744 238745 238746
   238747 238748 238749 238750 238751 238752 238753 238754 238755 238756
   238757 238758 238819 238820 238821 238822 238823 238824 238825 238826
   238827 238828 238829 238830 238831 238832 238833 238834 238835 238836
   238837 238838 238839 238900 238901 238902 238903 238904 238905 238906
   238907 238908 238909 238910 238911 238912 238913 238914 238915 238916
   238917 238918 238919 238920 238981 238982 238983 238984 238985 238986
   238987 238988 238989 238990 238991 238992 238993 238994 238995 238996
   238997 238998 238999 239000 239001 239062 239063 239064 239065 239066
   239067 239068 239069 239070 239071 239072 239073 239074 239075 239076
   239077 239078 239079 239080 239081 239082 239143 239144 239145 239146
   239147 239148 239149 239150 239151 239152 239153 239154 239155 239156
   239157 239158 239159 239160 239161 239162 239163 239224 239225 239226
   239227 239228 239229 239230 239231 239232 239233 239234 239235 239236
   239237 239238 239239 239240 239241 239242 239243 239244 239305 239306
   239307 239308 239309 239310 239311 239312 239313 239314 239315 239316
   239317 239318 239319 239320 239321 239322 239323 239324 239325 239386
   239387 239388 239389 239390 239391 239392 239393 239394 239395 239396
   239397 239398 239399 239400 239401 239402 239403 239404 239405 239406
   239467 239468 239469 239470 239471 239472 239473 239474 239475 239476
   239477 239478 239479 239480 239481 239482 239483 239484 239485 239486
   239487 239548 239549 239550 239551 239552 239553 239554 239555 239556
   239557 239558 239559 239560 239561 239562 239563 239564 239565 239566
   239567 239568 239629 239630 239631 239632 239633 239634 239635 239636
   239637 239638 239639 239640 239641 239642 239643 239644 239645 239646
   239647 239648 239649 239710 239711 239712 239713 239714 239715 239716
   239717 239718 239719 239720 239721 239722 239723 239724 239725 239726
   239727 239728 239729 239730 239791 239792 239793 239794 239795 239796
   239797 239798 239799 239800 239801 239802 239803 239804 239805 239806
   239807 239808 239809 239810 239811 239872 239873 239874 239875 239876
   239877 239878 239879 239880 239881 239882 239883 239884 239885 239886
   239887 239888 239889 239890 239891 239892 239953 239954 239955 239956
   239957 239958 239959 239960 239961 239962 239963 239964 239965 239966
   239967 239968 239969 239970 239971 239972 239973 240034 240035 240036
   240037 240038 240039 240040 240041 240042 240043 240044 240045 240046
   240047 240048 240049 240050 240051 240052 240053 240054 240115 240116
   240117 240118 240119 240120 240121 240122 240123 240124 240125 240126
   240127 240128 240129 240130 240131 240132 240133 240134 240135 240196
   240197 240198 240199 240200 240201 240202 240203 240204 240205 240206
   240207 240208 240209 240210 240211 240212 240213 240214 240215 240216
   240277 240278 240279 240280 240281 240282 240283 240284 240285 240286
   240287 240288 240289 240290 240291 240292 240293 240294 240295 240296
   240297]]]
p_source_input [[[ 0.0000000e+00  0.0000000e+00  0.0000000e+00 ...  0.0000000e+00
    0.0000000e+00  0.0000000e+00]
  [ 4.7698427e-07 -5.4399959e-07  1.2630575e-06 ...  1.2630575e-06
   -5.4399953e-07  4.7698416e-07]
  [ 8.3596893e-07 -9.5342102e-07  2.2136514e-06 ...  2.2136514e-06
   -9.5342074e-07  8.3596871e-07]
  ...
  [-9.4164091e-07  1.0739396e-06 -2.4934714e-06 ... -2.4934714e-06
    1.0739394e-06 -9.4164062e-07]
  [-6.7777000e-07  7.7299541e-07 -1.7947394e-06 ... -1.7947394e-06
    7.7299524e-07 -6.7776983e-07]
  [-2.4622784e-07  2.8082235e-07 -6.5201289e-07 ... -6.5201289e-07
    2.8082229e-07 -2.4622776e-07]]]
p_source_many [[[1]]]
p_source_mode [[[2]]]
pml_x_alpha [[[2.]]]
pml_x_size [[[30]]]
pml_y_alpha [[[2.]]]
pml_y_size [[[30]]]
pml_z_alpha [[[2.]]]
pml_z_size [[[34]]]
rho0

However the outputs are different on my linux system versus Andrew's mac system. Linux:

output from kwave (linux) 
Nt [[[3]]]
Nx [[[81]]]
Ny [[[81]]]
Nz [[[81]]]
absorbing_flag [[[1]]]
axisymmetric_flag [[[0]]]
c_ref [[[1500.]]]
dt [[[2.e-07]]]
dx [[[0.001]]]
dy [[[0.001]]]
dz [[[0.001]]]
nonlinear_flag [[[0]]]
nonuniform_grid_flag [[[0]]]
p0_source_flag [[[0]]]
p_max [[[0.         0.10062483 0.         ... 0.         0.00369736 0.        ]]]
p_min [[[-0.10217476  0.         -0.266634   ... -0.00996832  0.
   -0.0038342 ]]]
p_source_flag [[[63]]]
p_source_many [[[1]]]
p_source_mode [[[2]]]
pml_x_alpha [[[2.]]]
pml_x_size [[[30]]]
pml_y_alpha [[[2.]]]
pml_y_size [[[30]]]
pml_z_alpha [[[2.]]]
pml_z_size [[[34]]]
t_index [[[3]]]
transducer_source_flag [[[0]]]
ux_source_flag [[[0]]]
uy_source_flag [[[0]]]
uz_source_flag [[[0]]]

Mac:

output from kwave (mac) 
Nt [[[3]]]
Nx [[[81]]]
Ny [[[81]]]
Nz [[[81]]]
absorbing_flag [[[1]]]
axisymmetric_flag [[[0]]]
c_ref [[[1500.]]]
dt [[[2.e-07]]]
dx [[[0.001]]]
dy [[[0.001]]]
dz [[[0.001]]]
nonlinear_flag [[[0]]]
nonuniform_grid_flag [[[0]]]
p0_source_flag [[[0]]]
p_max [[[-3.4028235e+38 -3.4028235e+38 -3.4028235e+38 ... -3.4028235e+38
   -3.4028235e+38 -3.4028235e+38]]]
p_min [[[3.4028235e+38 3.4028235e+38 3.4028235e+38 ... 3.4028235e+38
   3.4028235e+38 3.4028235e+38]]]
p_source_flag [[[63]]]
p_source_many [[[1]]]
p_source_mode [[[2]]]
pml_x_alpha [[[2.]]]
pml_x_size [[[30]]]
pml_y_alpha [[[2.]]]
pml_y_size [[[30]]]
pml_z_alpha [[[2.]]]
pml_z_size [[[34]]]
t_index [[[3]]]
transducer_source_flag [[[0]]]
ux_source_flag [[[0]]]
uy_source_flag [[[0]]]
uz_source_flag [[[0]]]

So exactly the same input to the binary, but two different outputs. You can see the unreasonable values for p_max and p_min in the mac output.

We tried a few things like adding more time steps and pushing the target out further, but we still get the overflow on mac.

@peterhollender When you look at the raw input to kwave above, does anything stand out to you as causing an unreasonable simulation?

@peterhollender
Copy link
Contributor Author

peterhollender commented Jun 9, 2025

Peter we've looked at the macos overflow issue and narrowed it down to the kwave binary doing different things on different systems.

In all cases, the input file that gets delivered into the kwave binary looks like this (this is a print so arrays can get elided, but hopefully enough of the idea is there to understand):

input to kwave
However the outputs are different on my linux system versus Andrew's mac system. Linux:

output from kwave (linux)
Mac:

output from kwave (mac)
So exactly the same input to the binary, but two different outputs. You can see the unreasonable values for p_max and p_min in the mac output.

We tried a few things like adding more time steps and pushing the target out further, but we still get the overflow on mac.

@peterhollender When you look at the raw input to kwave above, does anything stand out to you as causing an unreasonable simulation?
Not really, but it's a very reductive simulation, so maybe something is coming up as zeros. Tried updating some of the sim parameter to explicitly use zero-delays.

@peterhollender
Copy link
Contributor Author

peterhollender commented Jun 9, 2025

Still failing, but mac is not on our "must support" list. Maybe it's a bug with the kwave binaries on mac. Have we seen normal output from them on mac with full-sized sims? Either way, this needs to get merged

@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented Jun 9, 2025

Have we seen normal output from them on mac with full-sized sims?

Yes, we have! The normal simulations work just fine.

I'll get this merged soon, skipping test_sim if I have to

@ebrahimebrahim ebrahimebrahim self-requested a review June 9, 2025 20:38
@ebrahimebrahim
Copy link
Collaborator

ebrahimebrahim commented Jun 9, 2025

Heads up that I am going to rebase this

@ebrahimebrahim ebrahimebrahim force-pushed the 258-k-wave-simulation-ignore-heterogenous-params branch from 77cd1d2 to 37758b2 Compare June 9, 2025 20:39
@ebrahimebrahim ebrahimebrahim force-pushed the 258-k-wave-simulation-ignore-heterogenous-params branch from 37758b2 to 32fef08 Compare June 9, 2025 20:40
@ebrahimebrahim ebrahimebrahim mentioned this pull request Jun 9, 2025
Open
@ebrahimebrahim ebrahimebrahim enabled auto-merge (rebase) June 9, 2025 20:53
ebrahimebrahim
ebrahimebrahim approved these changes Jun 9, 2025
View reviewed changes
Copy link
Collaborator

@ebrahimebrahim ebrahimebrahim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested on SlicerOpenLIFU main -- no issues with computing and using a solution!

I opened a separate bug #339 for the kwave macos thing, and marked the test_sim test to be skipped on mac only for now.

@ebrahimebrahim ebrahimebrahim merged commit bdf0b5e into main Jun 9, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebrahimebrahim ebrahimebrahim ebrahimebrahim approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

k-Wave simulation ignore heterogenous params

4 participants

@peterhollender @ebrahimebrahim @arhowe00