search

waltsims / k-wave-python

A Python interface to k-Wave GPU accelerated binaries
https://k-wave-python.readthedocs.io/en/latest/
GNU General Public License v3.0
101 stars 30 forks source link

Out-of-the-box usage #192

Closed talg2324 closed 8 months ago

talg2324 commented 1 year ago

Hey guys, awesome project. Really enjoying working with it. I have a few small suggestions for you that I think would improve the out-of-the-box usage. I'd rather not fork your project as I'd like to keep benefiting from updates you make.

  1. Switch all your internal printing with logging.log - running any simulation spams my console and I can't disable it without commenting out your code
  2. In the Executor class you run the kwave binaries with os.system- this isn't a good practice and it doesn't let you hide the kwave outputs either. I know you have kwave's native verbose_level as an argument in the SimulationExecution class, but have you thought about adding a more severe verbosity to hide all C++ outputs? Maybe replace os.system with: proc = subprocess.run(command, stdout=subprocess.DEVNULL, shell=True) return_code = proc.returncode
  3. Friendy reminder that linux=True is hard-coded in some places in the codebase
  4. I must say I was not able to figure out what is going on in the build_channel_data function. Several hard-coded values there. Maybe I didn't look hard enough.
  5. Seems like it would be pretty easy to update the beamform function to be more generic

PS- did I understand correctly that the cartesian dimensions (x, y, z) in the simulation grid correspond to the ultrasound imaging directions (z, x, y)?

Thanks anyway and again great work.

faridyagubbayli commented 1 year ago

Hey! Great points, thank you for taking time to share them with us! We will make sure to discuss them in the next discussion meeting (happens on Thursdays).

For the PS-part, I think @waltsims can comment the best.

waltsims commented 1 year ago

Hey,

Thanks for the feedback! On points 4/5 build_channel_data and the beamforming code are currently a catch-all for reconstruction code. k-wave-python was only intended to support running simulations, and not to be a beamforming project. In order to have an example early on, we hid the beamforming code in build_channel_data which depends on another project, uff-reader. I agree this part of the code could be improved but I think it would better to house ultrasound reconstruction in another project. I expect to see some new projects being published this conference season that might do the trick.

As far as the as the imaging coordinate directions, I'm not sure if I understand correctly, but I'll give it a shot. In k-wave, x is defined as the axial direction, or the direction of wave propagation, y is defined as the azimuthal direction across the face of the transducer, and z is defined as the elevational direction. A diagram can be found on page 79 of the k-wave-toolbox user manual.

image

You might be referring to some of the code in build_channel_data function which as previously mentioned used the uff axis convention?

I hope this helps. I agree with many of your suggestions and will try to merge them in. If you want to contribute, a PR would be welcomed as @faridyagubbayli already mentioned.

Best, Walter

waltsims commented 1 year ago

Thanks for PR #196, which addressed many of these suggestions. The beamforming could still stand to be updated.