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

issues with (P)OSMAPOSL sensitivity division #906

Open
KrisThielemans opened this issue Jul 8, 2021 · 6 comments
Open

issues with (P)OSMAPOSL sensitivity division #906

KrisThielemans opened this issue Jul 8, 2021 · 6 comments
Labels

Comments

@KrisThielemans
Copy link
Collaborator

spotted by @AnderBiguri

We use the divide function to divide by sensitivity

divide(multiplicative_update_image_ptr->begin_all(),
multiplicative_update_image_ptr->end_all(),
sensitivity.begin_all(),
small_num);

with small_num initialised here

static const float small_num = 0.000001F;

actual division code is in
https://github.com/UCL/STIR/blob/master/src/include/stir/numerics/divide.inl

This finds a threshold based on the max in the estimate. This is fine for normal images, but is dangerous for the parametric case, as the images can have very different scale. Therefore, the threshold could be inappropriate for the one of the images, resulting incorrect zeroes in the image.

@KrisThielemans
Copy link
Collaborator Author

We never see this in the logs due to #905

@KrisThielemans
Copy link
Collaborator Author

Of course, not so clear how to change the code (while still remaining generic), i.e. how do you find what is "small"?

@KrisThielemans
Copy link
Collaborator Author

the divide strategy seems in any case dangerous as it assumes that numerator and denominator are the same scale. (it should arguably use different small values for both of them)

@KrisThielemans
Copy link
Collaborator Author

I think that for the sensitivity division, we could/should set small_num to zero. (the main question here is if the backprojection updates a voxel a not. If not, image is undefined there, and we set it to 0.)

Less clear what to do in the OSL code.

@AnderBiguri
Copy link
Collaborator

In my hand-made version, indeed what I do to avoid this is to remove NaNs and Infs after the division, and nothing else. This makes the bug dissapear. That would work, not sure if its the ideal solution though.

@KrisThielemans
Copy link
Collaborator Author

the divide strategy seems in any case dangerous as it assumes that numerator and denominator are the same scale. (it should arguably use different small values for both of them)

The divide strategy is used to divide the backproj(quotient) by backproj(1) (i.e. subset-sensitivity), so these should (after a while) have similar scales, so this is probably ok.

However, it sets the 0/0 to 0, but this is inappropriate for OSEM. It should set it to 1 (as we should not update voxel values which are not affected by the subset).

I still have to think what we need to do with OSL (which is a whole load of other difficulties and heuristics)

@KrisThielemans KrisThielemans changed the title parametric POSMAPOSL potential danger in sensitivity division issues with (P)OSMAPOSL sensitivity division Jul 15, 2021
KrisThielemans added a commit to KrisThielemans/STIR-1 that referenced this issue Jan 5, 2022
We used a small threshold to detect 0/0 when dividing by the
sensitivity image in the Poisson log likelihood. However, this threshold
was problematic, and actually caused problems in parametric imaging
due to different scales of the parametric images.

We now set that threshold to zero.

Addresses UCL#906
KrisThielemans added a commit that referenced this issue Jan 5, 2022
remove threshold in division by sensitivity

See #906 (although this doesn't fix it completely)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants