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

fixes STIR TOF AcquisitionData algebra #1208

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

KrisThielemans
Copy link
Member

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.

Copy link
Contributor

@evgueni-ovtchinnikov evgueni-ovtchinnikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be backward compatible? If not, should this not be merged after data-cont-templ?

#define TOF_LOOP for (int k=data()->get_min_tof_pos_num(); k<=data()->get_max_tof_pos_num(); ++k)
#define TOF_ARG , k
#else
#define TOF_LOOP
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

#define TOF_LOOP(k) for(int k=...)
#define TOF_ARG(k) , k
#else
#define TOF_LOOP(k)
#define TOF_ARG(k)

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
Copy link
Member Author

KrisThielemans commented Aug 21, 2023

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 KrisThielemans linked an issue Aug 21, 2023 that may be closed by this pull request
@KrisThielemans
Copy link
Member Author

@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

test.check_if_equal(1, abs(t - s) <= eps * abs(t))

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
Copy link
Member Author

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
Copy link
Contributor

can you please try

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

in norm() instead of

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

- increase tolerance for norm/dot tests, as large data sizes give larger error
- change tests calls to use check_if_*tolerance to be more informative when
the test fails
- remove some unnecessary as_array() calls, speeding it up a little bit
@KrisThielemans
Copy link
Member Author

@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 KrisThielemans merged commit 60bda8a into SyneRBI:master Aug 22, 2023
4 checks passed
@KrisThielemans KrisThielemans deleted the fixTOFalgebra branch August 22, 2023 10:38
@KrisThielemans
Copy link
Member Author

@NicoleJurjew, this is now merged to SIRF master

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 this pull request may close these issues.

port STIR TOF support to SIRF
2 participants