Skip to content
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

Added downsampling of BlocksOnCylindrical scanners in scatter #1291

Conversation

markus-jehl
Copy link
Contributor

@markus-jehl markus-jehl commented Nov 24, 2023

Changes in this pull request

Added the functionality to downsample BlocksOnCylindrical scanners in views and detectors per ring during scatter simulation, in order to speed up the computations. The default behaviour did not change.

Testing performed

This was tested with iterative scatter estimation and simulation on NeuroLF and BPET blocks geometry and compared to non-downsampled scatter estimation. The differences were of the order of 1% when the crystals per ring and views were both downsampled by a factor of 2.

Related issues

fixes #1290
fixes #1393
fixes #1497

Checklist before requesting a review

  • I have performed a self-review of my code
  • [] I have added docstrings/doxygen in line with the guidance in the developer guide
  • I have implemented unit tests that cover any new or modified functionality (if applicable)
  • The code builds and runs on my machine
  • documentation/release_XXX.md has been updated with any functionality change (if applicable)

@markus-jehl markus-jehl force-pushed the issue/1290-speeding-up-scatter-simulation-for-blocks-geometry branch from 5a4f139 to beeeea9 Compare November 28, 2023 12:22
@markus-jehl
Copy link
Contributor Author

markus-jehl commented Dec 13, 2023

original scatter:
image
below are plots of the differences between downsampled scatter and original scatter, relative to mean(abs(original_scatter))=11.5
downsampling factor 1.066667 (from 32 to 30 detectors per module)
image
downsampling factor 1.28 (from 32 to 25 per module)
image
downsampling factor 1.6 (from 32 to 20 per module)
image
downsampling factor 2 (from 32 to 16 per module)
image
downsampling factor 3.2 (from 32 to 10 per module)
image

@markus-jehl
Copy link
Contributor Author

markus-jehl commented Dec 13, 2023

image
relative difference between original scatter and downsampled scatter with factor 2 (divided by mean intensity of areas with intensity > 0.1 => 0.5)
image

@markus-jehl
Copy link
Contributor Author

Therefore it looks like choosing integer factors for the downsampling isn't required, and the intersections between the modules remain problematic (sometimes over 10% errors in the scatter estimate). However, on the final image the differences caused by the downsampled scatter are below 3% of the average hot values.

@markus-jehl
Copy link
Contributor Author

As part of this investigation, a bug was found: #1396
This should be fixed on the ProjData side, but it raises another problem with the scatter estimation code itself: for the last iteration we first upsample from the downsampled scanner to the full-size scanner here:

upsample_and_fit_scatter_estimate(*scaled_est_projdata_sptr,
*data_to_fit_projdata_sptr,
*unscaled_est_projdata_sptr,
*normalisation_factors_sptr,
*this->mask_projdata_sptr,
local_min_scale_value,
local_max_scale_value,
this->half_filter_width,
spline_type,
true);

And then we run upsample_and_fit_scatter_estimate a second time here:
upsample_and_fit_scatter_estimate(*scatter_estimate_sptr,
*this->input_projdata_sptr,
*temp_projdata,
*normalisation_factors_3d_sptr,
*this->input_projdata_sptr,
1.0f,
1.0f,
1,
spline_type,
false);

The second call will fail once #1396 is fixed, since the input ProjData are correctly identified as not homogeneously sampled in axial direction. However, we also don't need the second interpolation step, because input and output ProjData have the same dimensions. Therefore, I would propose to replace that call with a simple inverse_SSRB followed by scatter_normalisation.undo(). This is all that is needed at this point.

Copy link
Collaborator

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

I'm hoping that once this is done, we can avoid the almost repeated code for the blocks and cylindrical case, but maybe do that in stages.

src/include/stir/scatter/ScatterEstimation.h Show resolved Hide resolved
src/scatter_buildblock/ScatterEstimation.cxx Outdated Show resolved Hide resolved
src/include/stir/scatter/ScatterEstimation.h Outdated Show resolved Hide resolved
@markus-jehl
Copy link
Contributor Author

I'm hoping that once this is done, we can avoid the almost repeated code for the blocks and cylindrical case, but maybe do that in stages.

Do you mean in interpolate_projdata? This would indeed be very nice. I'll have a look at it!

@KrisThielemans
Copy link
Collaborator

I'm hoping that once this is done, we can avoid the almost repeated code for the blocks and cylindrical case, but maybe do that in stages.

Do you mean in interpolate_projdata? This would indeed be very nice. I'll have a look at it!

there's some in the ray-tracing projector as well, but that's on our wish-list :-)

@KrisThielemans
Copy link
Collaborator

The second call will fail once #1396 is fixed, since the input ProjData are correctly identified as not homogeneously sampled in axial direction.

ok, although we could at least in principle make that check more sophisticated to see if needs to interpolate or not, but I'm fine with not doing that!

However, we also don't need the second interpolation step, because input and output ProjData have the same dimensions. Therefore, I would propose to replace that call with a simple inverse_SSRB followed by scatter_normalisation.undo(). This is all that is needed at this point.

good point!

@markus-jehl
Copy link
Contributor Author

On a simulated cylinder, the scatter estimation has much smaller errors when we use linear interpolation instead of quadratic interpolation. I strongly suspect that this is because we quite aggressively downsample the scanner (e.g. 8 instead of 48 rings) to speed up the computation time. Quadratic interpolation simply can't cope with that level of simplification. Below is the quadratic interpolation vs. ground truth first, then the linear interpolation vs. ground truth:
image
image

@markus-jehl markus-jehl marked this pull request as draft June 13, 2024 13:26
@markus-jehl markus-jehl marked this pull request as draft June 13, 2024 13:26
Markus Jehl added 8 commits July 4, 2024 09:50
…ng-up-scatter-simulation-for-blocks-geometry
… a per module basis and then bilinear interpolation to find the best view/tangential position for the floating point crystal IDs.
…instead fill in the interpolated values directly.
@markus-jehl markus-jehl marked this pull request as ready for review July 17, 2024 08:43
@markus-jehl
Copy link
Contributor Author

Below is the difference of the downsampled (half detectors per ring) scatter estimation and the ground truth scatter followed by the non-downsampled scatter estimation. The scatter amplitude is a little over 8, so in both cases the errors are <2%.
image
And here is the same comparison with the reconstructed images (ground truth image has intensity 1, so the errors are usually below 1% inside the cylinder and only higher at the edge):
image

@markus-jehl
Copy link
Contributor Author

Plots as at the beginning of the PR: scatter for Hoffman phantom data with downsampling of 2 compared against no downsampling.
image
image

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>
Copy link
Collaborator

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

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

I haven't checked the actual interpolation code, but trust you! However, some documentation what the code is doing would be good, as it's quite hard to see.

src/scatter_buildblock/ScatterEstimation.cxx Show resolved Hide resolved
src/scatter_buildblock/ScatterSimulation.cxx Show resolved Hide resolved
documentation/release_6.2.htm Outdated Show resolved Hide resolved
src/buildblock/interpolate_projdata.cxx Outdated Show resolved Hide resolved
src/buildblock/interpolate_projdata.cxx Outdated Show resolved Hide resolved
src/buildblock/interpolate_projdata.cxx Outdated Show resolved Hide resolved
@markus-jehl
Copy link
Contributor Author

I haven't checked the actual interpolation code, but trust you! However, some documentation what the code is doing would be good, as it's quite hard to see.

Ok, I will add an overall description.

@KrisThielemans KrisThielemans merged commit 33a0106 into UCL:master Aug 30, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants