fixes STIR TOF AcquisitionData algebra

SyneRBI / SIRF

Main repository for the CCP SynerBI software

http://www.ccpsynerbi.ac.uk

Other

58 stars 29 forks source link

fixes STIR TOF AcquisitionData algebra #1208

Closed KrisThielemans closed 10 months ago

KrisThielemans commented 10 months ago

inserted TOF loops. However, this needs the STIR_TOF preprocessor variable to be defined, which is still to be merged on the STIR TOF branch.

KrisThielemans commented 10 months ago

It should be once I'm done (had some stupid errors!). All changes should be within #ifdef STIR_TOF. If you don't build with it, everything should be identical.

KrisThielemans commented 10 months ago

@evgueni-ovtchinnikov currently some of the tests fail due to numerical rounding error. Printing stuff like

 print('norm:', s, t, eps, abs(t-s), eps*abs(t))

after https://github.com/SyneRBI/SIRF/blob/1bb1b111a80f48df5e491a8ee9201715d4e9db42/src/common/Utilities.py#L600

I get

+++ test 0 failed: expected 1, got False
norm: 47163.3984375 47161.4 1e-05 2.0 0.471613984375
+++ test 1 passed
+++ test 2 passed
+++ test 3 failed: expected 1, got False
dot: 68452680.0 68447064.0 1e-05 5616.0 684.47064
+++ test 4 passed
+++ test 5 passed

etc (2 more tests are failing, not sure yet which).

Your implementations look ok though, they use double for the temp variable (see e.g. here). I guess we could increase our tolerance a bit?

Aside from that, it's annoying that these tests don't give any feedback about failure and I had to add the print by hand. Easy to replace with check_if_equal_within_tolerance`?

KrisThielemans commented 10 months ago

note that the above failure is on some TOF data (I haven't run on non-TOF yet, but Actions will tell us)

evgueni-ovtchinnikov commented 10 months ago

can you please try

    return (float)std::sqrt(t);

in norm() instead of

    return std::sqrt((float)t);

KrisThielemans commented 10 months ago

@evgueni-ovtchinnikov that does indeed look better but it didn't make any practical difference. It could indeed be that the numpu.linalg.norm isn't as smart as we are 😄 . In any case, the difference is well below our precision.

I've now

modified the tests to
- relax it for norm and dot
- use check_if_..._tolerance for clearer output if they fail.
changed norm to do what you suggest, but I also rewrote the loop, such that it's far shorter. Finally, I used stir::norm_squared to save 2 lines (sadly had to modify an unrelated line using stir::norm due to a STIR bug). This could be done for the others as well, but I have run out of time.

I'm done with this therefore and would like to merge. Can you have a look please?

KrisThielemans commented 10 months ago

@nicolejurjew, this is now merged to SIRF master