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

Fix SSRB for ACFs in scatter estimation #1531

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rmapree
Copy link

@rmapree rmapree commented Nov 5, 2024

Changes in this pull request

Modified the make_2D_projdata_sptr function to include boolean. Added this into acfs modification to prevent converting 3d acfs to 2d.

Testing performed

rebuilt this, also used recon_test_pack tests to check scatter estimation. This works with these changes.

Related issues

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)

@KrisThielemans
Copy link
Collaborator

Looks good! Great to hear that it works!

The pre-commit checks fail due to white-space conventions.
https://github.com/UCL/STIR/actions/runs/11686408807/job/32542097224
If you're on Ubuntu 22.04, the easiest would be to

sudo apt install clang-format
clang-format -i include/stir/scatter/ScatterEstimation.h
clang-formiat -i scatter_buildblock/ScatterEstimation.cxx

If on another OS/version, we need a specific clang-format version, e.g. via conda.

There's more info on https://github.com/UCL/STIR/blob/master/documentation/devel/git-hooks.md, but it's a bit too brief...

@KrisThielemans KrisThielemans added this to the v6.3 milestone Nov 5, 2024
@KrisThielemans KrisThielemans self-assigned this Nov 5, 2024
@KrisThielemans
Copy link
Collaborator

Hopefully fixes #1280

@KrisThielemans
Copy link
Collaborator

Hi Toni, could you update the documentation/release_6.3.htm please? Add something in the "bug fixes" section, referring to this PR. May be text like (feel free to improve!)

Fixed a bug in the scatter estimation code (introduced in release 5.1.0) if input data is 3D and "cylindrical" (there was no bug for "blocksoncylindrical" data). The scatter estimation runs on data constructed via SSRB. However, the attenuation correction factors were incorrectly obtained with adding of oblique segments (as opposed to averaging). This resulted in intermediate images that had the wrong attenuation correction. This was compensated by the tail-fitting, but resulted in unexpected scale facctors (roughly smaller with a factor num_segments than expected). This means that if you used the "min/max scale factor" feature in the scatter estimate, you will have to adjust your threshold values. Expected scatter tail-fitting scale factors should now be restored to ~1-1.5 (depending on the amount of multiple and out-of-FOV scatter). See Issue 1532 for more detail.

Once done, tick the box at the start of the issue (looks like you'll need to edit it).

@KrisThielemans KrisThielemans changed the title Scatter Estimation - boolean for 2d proj data - acfs 3d Fix SSRB for ACFs in scatter estimation Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in scatter estimation when using SSRB for attenuation factors
2 participants