search

IMSY-DKFZ / simpa

The Simulation and Image Processing for Photoacoustic Imaging (SIMPA) toolkit.
https://simpa.readthedocs.io/en/main/
Other
65 stars 17 forks source link

T314 remove fillers #315

Closed frisograce closed 3 days ago

frisograce commented 4 days ago

Please check the following before creating the pull request (PR):

jgroehl commented 4 days ago

Hello!

I'm not quite sure I understand the changes. I agree that the "filler" argument bloats the interface and implementation but it was a necessary change. As soon as heterogeneities are introduced, the blood volume fraction per voxel is not strictly set to 1 any more and can be less than 1 because of the random nature of the heterogeneity maps.

In this PR, I do not see an alternative way to compensate for this issue. Perhaps I just missed it...?

frisograce commented 4 days ago

Hi!

Sorry, I'm not quite understanding the question. Why would we always want blood volume fractions to be one? As surely everywhere except the blood vessels will have a blood volume less than 1? In regards to total volume, this of course must always add to one I agree. However, one thing I have now seen and agree with is that if the volume fraction given by the user is too big in muscle fraction etc, it will make

.append(value=MOLECULE_LIBRARY.muscle_scatterer(volume_fraction=1 - fraction_oxy - fraction_deoxy - water_volume_fraction)

not work, as the volume fraction will become negative. This will happen regardless of these changes, but perhaps something to consider. An error should be created here to specify to the user what precisely has gone wrong.

It does also speak to how the two methods work. It shows how when removing filler, we end up with the 'excess' volume occupied by the scatterer. With the filler, we end up with a slightly strange occurrence. Here, we specify the custom water volume with a constant again, and then fill the rest with the muscle_scatterer. However, we then go on to add the custom water as a filler, however the only value it can take is still the constant water volume fraction as this will be the only volume left to occupy. Therefore it creates more computation rather unnecessarily.

I hope this might help clarify some of your question, but be sure to let me know if/what I have misunderstood.

jgroehl commented 4 days ago

Oh sorry, I chose the wrong words regarding volume fractions and blood volume fractions.. Of course - as soon as one of the fractions is an array, the entire expression is an array that works seamlessly. I think I have just missed that in the early implementation!

frisograce commented 3 days ago

No worries, I completely missed it too! It was only when optimising that i saw it. Will add a check :)