search

SyneRBI / SIRF

Main repository for the CCP SynerBI software
http://www.ccpsynerbi.ac.uk
Other
57 stars 29 forks source link

Integer valued sirf.ObjectiveFunction.value #1170

Open Imraj-Singh opened 1 year ago

Imraj-Singh commented 1 year ago

I am reconstructing with SIRF and have only encountered integer values for sirf.ObjectiveFunction.value.

Is this expected behaviour? I could be doing something wrong, but I am not sure I am... I had a look at how it is exposed in SIRF and it seems that a float type is used, but all my values are in the format "1234567.0" with the values after the decimal non-existent...

Help would be appreciated!

KrisThielemans commented 1 year ago

this is obviously a bit strange, but floats do have limited precision, and there could be some printing stuff going on.

Note that STIR uses doubles for the objective function, so if SIRF uses floats, we'd loose some precision there.

Imraj-Singh commented 1 year ago

I have managed to reproduce this problem in the ML_reconstruction.ipynb notebook in the pet SIRF-Exercises.

For the acquired_data in the notebook I multiply by an arbitrary value i.e. "100000"

image

In the reconstruction I print the objective function value:

image

As you can see there is a problem with precision it seems. I am a bit worried if the could affect gradients etc?

KrisThielemans commented 1 year ago

As you can see there is a problem with precision it seems.

Do I?

Yes, the increment is small. there's a large (and in a sense arbitrary) offset to the KL. @robbietuk created an issue https://github.com/UCL/STIR/issues/697 on the offset and had a PR but ran out of steam https://github.com/UCL/STIR/pull/760.

I am a bit worried if the could affect gradients etc?

the computation of the gradient is independent of the computation of the value. The latter is much more sensitive to numerical problems. (Adding lots of numbers with limited precision is tricky).

Do check if SIRF uses doubles.

DANAJK commented 1 year ago

If you multiply by 1000 do you get the same effect of everything after the decimal point being zero? In that case we know it is wrong because we have seen there are more significant figures.

KrisThielemans commented 7 months ago

what's the status here please?

KrisThielemans commented 1 month ago

@Imraj-Singh this one is a bit urgent. Can you clarify if we need to fix anything?

Imraj-Singh commented 1 month ago

@KrisThielemans I am not sure if the precision loss due to the KL offset will have meaningful ramifications for assessing convergence etc? I did quickly look at the source and it seems that the STIR value is cast to float in SIRF: https://github.com/SyneRBI/SIRF/blob/6c7259d1e37f6112610619883c385325b2998c63/src/xSTIR/cSTIR/cstir.cpp#L1152

evgueni-ovtchinnikov commented 2 weeks ago

Fixed now - see #1264. @Imraj-Singh Please try again.