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

ML norm code should work by bucket, not by block #838

Open
KrisThielemans opened this issue Mar 2, 2021 · 4 comments · Fixed by #940
Open

ML norm code should work by bucket, not by block #838

KrisThielemans opened this issue Mar 2, 2021 · 4 comments · Fixed by #940

Comments

@KrisThielemans
Copy link
Collaborator

As found by @NikEfth, @francescaleek and @emikhaylova, the current assumption behind symmetries in find_ML_normfactors3D that the scanner is invariant over rotations by block is often incorrect in practice. Most scanners will have a few blocks in a "bucket", and the rotational (and for some scanners even axial translation) symmetry should be applied on the bucket level.

If the symmetries are wrong, @francescaleek found that the patterns in the estimated geometric factors will incorrect (mostly underestimated).

@robbietuk
Copy link
Collaborator

robbietuk commented Mar 12, 2021

Some comments:

  • ML norm code currently uses num_crystals_per_block, it should use num_crystals_per_bucket instead
  • The handling of virtual crystals in a block/bucket configuration might take some thinking about.
    • Both in trans-axial and axial directions

@robbietuk
Copy link
Collaborator

I have given this a shot and while I thought it would be nice and easy, there are ~180 uses of foobar_per_block mentions that simply need to be replaced with foobar_per_bucket. Therefore, I will wait for #833 to be merged before I fix this problem otherwise the conflicts will be significant and confusing

robbietuk added a commit to robbietuk/STIR that referenced this issue Mar 15, 2021
This is a hard set of change to a lot of variables, but not all. Therefore, I will save this for later. See UCL#838
@KrisThielemans
Copy link
Collaborator Author

#940 solves this without the renaming. That still needs to be done.

KrisThielemans added a commit that referenced this issue Nov 29, 2021
Super-block versions of find_ML_normfactors3D.cxx and apply_normfactors3D.cxx

Fixes #838
@KrisThielemans
Copy link
Collaborator Author

re-opened such that we don't forget to rename all the internal functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants