Modifications for psmr2024 #1430

NikEfth · 2024-05-15T19:02:55Z

Changes in this pull request

Testing performed

Related issues

Checklist before requesting a review

[] I have performed a self-review of my code
[] I have added docstrings/doxygen in line with the guidance in the developer guide
[] I have implemented unit tests that cover any new or modified functionality (if applicable)
[] The code builds and runs on my machine
[] documentation/release_XXX.md has been updated with any functionality change (if applicable)

A single TOF scanner can be used in the calculation of the sensitivity image, while applying the boundaries of the coincidence window.

* Building upon the cached lm reconstruction, we further parallelize the calculation of the sensitity image to muliple passes when the scanner is long. * Now, a scanner is TOF ready even if it does not have set timing resolution. If the timing resolution is 0 ps or if the TOF mashing leads to a scanner with 1 TOF bin then we apply boundaries on the LOR's length, equal to the coinc window. The probabilities do not change as we set them to 1. * cxx17 is nessesary for newer ROOT.

NikEfth · 2024-05-15T19:03:34Z

If this commit breaks many tests, I would consider it expected.

src/buildblock/ProjDataInfo.cxx

src/IO/InterfileHeader.cxx

KrisThielemans · 2024-05-15T20:23:29Z

src/include/stir/ProjDataInfo.h

-
+  //! Similar to create_non_tof_clone() but returns a single TOF version of the ProjDataInfo setting the TOF mashing factor sush that,
+  //! a single TOF bin remains. This is usefull to apply the coincidence window boundaries without affecting the probabilities.
+    inline shared_ptr<ProjDataInfo> create_single_tof_clone() const;


I suppose we need this for backwards compatibility? i.e. need to keep create_non_tof_clone unmodified? If so, I'd be tempted to add a bool to create_non_tof_clone() instead.

Note: will be obsolete once we merge another PR on SSRB for TOF

src/include/stir/ProjDataInfo.inl

src/include/stir/Scanner.h

KrisThielemans · 2024-05-15T20:26:41Z

..._buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndListModeDataWithProjMatrixByBin.cxx

  info(boost::format("Calculating sensitivity for subset %1%") % subset_num);

  int min_timing_pos_num = use_tofsens ? this->proj_data_info_sptr->get_min_tof_pos_num() : 0;
  int max_timing_pos_num = use_tofsens ? this->proj_data_info_sptr->get_max_tof_pos_num() : 0;
-  if (min_timing_pos_num < 0 || max_timing_pos_num > 0)
+  if (min_timing_pos_num < 0 || max_timing_pos_num > 1)


Suggested change

if (min_timing_pos_num < 0 || max_timing_pos_num > 1)

if (min_timing_pos_num < 0 || max_timing_pos_num > 0)

No, this is correct. Up to one TOF bin should not be considered TOF reconstruction.

still not with you on this one (i.e. with the 1). Why not say num_timing_poss > 1

src/recon_buildblock/ProjMatrixByBin.cxx

Co-authored-by: Kris Thielemans <[email protected]>

KrisThielemans · 2024-05-16T08:08:58Z

tests fail with

ERROR: Non-TOF data with inconsistent Time-of-Flight bin number - Aborted operation.

NikEfth · 2024-05-16T14:01:31Z

We can turn this to a warning, for now.
Or, drop the old nonTOF altogether. But it will break people reading old headers.

KrisThielemans

A few more comments. Please merge master as CI is currently not running due to a conflict.

src/IO/InterfileHeader.cxx

KrisThielemans · 2024-05-16T18:14:53Z

src/include/stir/Scanner.h

+    \li \c If the user set number for TOF positions to 1 and a bin size equal to the
+          coincidence window then the reconstruction will essential be nonTOF but the
+            projector will restrict the size of the LOR to the size of the coincidence window.
+      \li \c The scanner will be classified as TOF enabled when the numer of TOF bins and
+TOF bin size are  >1 and >0, respectively. If the energy resolution is not set that will be fine
+as long as the final TOF possitions is 1. Then we just restict the size of the LOR.


Suggested change

\li \c If the user set number for TOF positions to 1 and a bin size equal to the

coincidence window then the reconstruction will essential be nonTOF but the

projector will restrict the size of the LOR to the size of the coincidence window.

\li \c The scanner will be classified as TOF enabled when the numer of TOF bins and

TOF bin size are >1 and >0, respectively. If the energy resolution is not set that will be fine

as long as the final TOF possitions is 1. Then we just restict the size of the LOR.

\li \c The scanner will be classified as TOF enabled when the number of TOF bins and

TOF bin size are >1 and >0, respectively.

\li \c If the user sets the number for TOF positions to 1, the TOF bin size should correspond to the size of the coincidence window. If the TOF timing resolution is zero, then the reconstruction will essentially be nonTOF but the

projector will restrict the length of the LOR according to the TOF bin width.

KrisThielemans · 2024-05-16T18:17:03Z

..._buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndListModeDataWithProjMatrixByBin.cxx

  info(boost::format("Calculating sensitivity for subset %1%") % subset_num);

  int min_timing_pos_num = use_tofsens ? this->proj_data_info_sptr->get_min_tof_pos_num() : 0;
  int max_timing_pos_num = use_tofsens ? this->proj_data_info_sptr->get_max_tof_pos_num() : 0;
-  if (min_timing_pos_num < 0 || max_timing_pos_num > 0)
+  if (min_timing_pos_num < 0 || max_timing_pos_num > 1)


still not with you on this one (i.e. with the 1). Why not say num_timing_poss > 1

CMakeLists.txt

…tion

Co-authored-by: Kris Thielemans <[email protected]>

NikEfth · 2024-05-16T22:10:03Z

A few more comments. Please merge master as CI is currently not running due to a conflict.

I hadn't noticed that ...

This will allow implementation specific tests for a particular prior.

- we were using abs() which (at least on some systems) converted to integers, giving the wrong result. now using std::abs() - introduce value/derivative_10 function for more concise code

now use INFINITY for second order derivatives, obtained by continuity

split the tests for the different priors in different classes, such that we can have specific tests for each. Currently, only added some for the RDP, including a Lipschitz test and limit for large values.

The RDP test was failing with eps=1.e-3, so now using 1.e-4

use double in neighbourhood calculations

Fixes #1411

previous version failed on macos-14-arm64. We have a very fragile list of checks. Presumably, CMake does a lot better, so use CMAKE_CXX_BYTE_ORDER in ByteOrderDefine (via config.h)

run clang-format on Ubuntu on ByteOrderDefine.h. Apparently, there was a version mismatch.

KrisThielemans · 2024-05-18T13:48:22Z

good catch. Another case for getting rid of this special handling relying on the generic one...

KrisThielemans · 2024-05-18T19:12:25Z

@markus-jehl you're probably not able to, but in case you can, this PR should speed up the ML_norm code a fair amount, so you could potentially try that. It'd be good to have some reassurance on real data, as opposed to our simple test.

use it as max

…smr2024' into modifications_for_psmr2024

KrisThielemans · 2024-05-27T07:51:57Z

recon_test_pack/root_header.hroot

@@ -14,6 +14,7 @@ View offset (degrees)					 := 0
 Maximum number of (unmashed) TOF time bins := 5
 Size of unmashed TOF time bins (ps)        := 820.0
 TOF timing resolution (ps)                 := 400.0
+TOF mashing factor:= 1


we shouldn't need to add this.

KrisThielemans · 2024-05-27T07:52:40Z

src/buildblock/FilePath.cxx

@@ -72,7 +72,7 @@ FilePath::is_directory() const
  struct stat info;

  if (stat(my_string.c_str(), &info) != 0)
-    error(boost::format("FilePath: Cannot access %1%.") % my_string);
+    warning(boost::format("FilePath: Cannot access %1%.") % my_string);


why did we make this change?

KrisThielemans · 2024-05-27T07:53:43Z

src/buildblock/ProjDataInfo.cxx

@@ -177,9 +177,46 @@ ProjDataInfo::set_tof_mash_factor(const int new_num)
    {
      tof_mash_factor = new_num;
      if (tof_mash_factor > scanner_ptr->get_max_num_timing_poss())
-        error("ProjDataInfo::set_tof_mash_factor: TOF mashing factor (" + std::to_string(tof_mash_factor)
+        {
+        warning("ProjDataInfo::set_tof_mash_factor: TOF mashing factor (" + std::to_string(tof_mash_factor)


I'd still want an error here

KrisThielemans · 2024-05-27T07:54:26Z

src/buildblock/ProjDataInfo.cxx

              + +") must be smaller than or equal to the scanner's number of max timing bins ("
              + std::to_string(scanner_ptr->get_max_num_timing_poss()) + ").");
+        }
+      else if (tof_mash_factor == scanner_ptr->get_max_num_timing_poss())


let's see what happens if we remove this special case, as it shouldn't be "special"

…smr2024

We need to preserve the TOF character, such that coincidence window etc is taken into account.

src/include/stir/Scanner.h

KrisThielemans · 2024-06-01T18:38:08Z

src/listmode_buildblock/CListModeDataROOT.cxx

+  // if (check_scanner_match_geometry(error_str, this_scanner_sptr) == Succeeded::no)
+  //   {
+  //     error(error_str.c_str());
+  //   }


will have to re-enable this. Why did you need it?

src/recon_buildblock/ML_estimate_component_based_normalisation.cxx

src/utilities/construct_fanProjData_fromProjData.cxx

KrisThielemans · 2024-06-01T18:57:54Z

src/utilities/CMakeLists.txt

@@ -21,6 +21,7 @@ set(${dir_EXE_SOURCES}
 	manip_projdata.cxx
 	display_projdata.cxx
 	do_linear_regression.cxx
+	construct_fanProjData_fromProjData


please rename this file to save_FanProjData_from_ProjData

KrisThielemans · 2024-06-01T19:50:32Z

Ubuntu clang job fails with

usr/bin/ld: CMakeFiles/test_BSplinesRegularGrid.dir/test_BSplinesRegularGrid.cxx.o: in function `.omp_outlined.':
test_BSplinesRegularGrid.cxx:(.text+0x39bf): undefined reference to `__atomic_load'
/usr/bin/ld: test_BSplinesRegularGrid.cxx:(.text+0x39f3): undefined reference to `__atomic_compare_exchange'
clang: error: linker command failed with exit code 1 (use -v to see invocation)usr/bin/ld: CMakeFiles/test_BSplinesRegularGrid.dir/test_BSplinesRegularGrid.cxx.o: in function `.omp_outlined.':
test_BSplinesRegularGrid.cxx:(.text+0x39bf): undefined reference to `__atomic_load'
/usr/bin/ld: test_BSplinesRegularGrid.cxx:(.text+0x39f3): undefined reference to `__atomic_compare_exchange'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Potentially related to https://gitlab.kitware.com/cmake/cmake/-/issues/23021 but it's weird tst_BSplines links ok.

KrisThielemans · 2024-06-01T21:20:53Z

test_ML_norm segfaults in the MacOS Debug job

KrisThielemans · 2024-06-09T03:35:18Z

Potentially related to https://gitlab.kitware.com/cmake/cmake/-/issues/23021 but it's weird tst_BSplines links ok.

Now https://gitlab.kitware.com/cmake/cmake/-/issues/26037

…smr2024

KrisThielemans · 2024-06-12T21:50:18Z

Linking problem was fixed in #1449 which has taken the sum/find_min/max parallelisation commits, and is now merged to master, so I've merged that back on here.

I suggest we split this PR in 2: one for the sensitivity calculation, and one for the parallelisation of the norm code. We can do that again by cherry-picking relevant commits. Maybe we can resolve things here first though, as you prefer.

NikEfth added 2 commits March 26, 2024 01:23

Adding support for singleTOF scanner.

9e246fd

A single TOF scanner can be used in the calculation of the sensitivity image, while applying the boundaries of the coincidence window.

NikEfth requested a review from KrisThielemans May 15, 2024 19:03

KrisThielemans requested changes May 15, 2024

View reviewed changes

NikEfth and others added 4 commits May 15, 2024 21:21

Update src/buildblock/ProjDataInfo.cxx

4ba55e6

Co-authored-by: Kris Thielemans <[email protected]>

Update src/include/stir/ProjDataInfo.inl

f5d663d

Co-authored-by: Kris Thielemans <[email protected]>

Update src/recon_buildblock/ProjMatrixByBin.cxx

438be2f

Co-authored-by: Kris Thielemans <[email protected]>

Minor fixes and comments.

0fb593d

KrisThielemans requested changes May 16, 2024

View reviewed changes

KrisThielemans reviewed May 16, 2024

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

NikEfth and others added 5 commits May 16, 2024 15:06

Minor fixes and executable to cache listmode files without reconstruc…

7bf7e52

…tion

Update CMakeLists.txt

6aa04b0

Co-authored-by: Kris Thielemans <[email protected]>

Update src/IO/InterfileHeader.cxx

5fa949c

Co-authored-by: Kris Thielemans <[email protected]>

Minor fixes

e67faec

Merge branch 'master' into modifications_for_psmr2024

e7a2656

KrisThielemans added 11 commits May 16, 2024 18:22

Split GeneralisedPriorTests into several classes

9179482

This will allow implementation specific tests for a particular prior.

fix abs() bug for RDP and slightly cleaner code

f7c15e6

- we were using abs() which (at least on some systems) converted to integers, giving the wrong result. now using std::abs() - introduce value/derivative_10 function for more concise code

change RDP Hessian behaviour at 0,0 when epsilon==0

a9c3ddb

now use INFINITY for second order derivatives, obtained by continuity

extra tests for the Relative Difference Prior

005acb2

split the tests for the different priors in different classes, such that we can have specific tests for each. Currently, only added some for the RDP, including a Lipschitz test and limit for large values.

make epsilon in numerical test for prior gradient a bit smaller

b7f727d

The RDP test was failing with eps=1.e-3, so now using 1.e-4

fix and document RDP large limit test for numerical error

d8b3826

update RDP doc [ci skip]

f17a7b0

increase numerical precision of priors

619cc86

use double in neighbourhood calculations

[GHA] remove explicit setting of xcode version

32dc591

Fixes #1411

update Byte Order compile-time checks

fa21161

previous version failed on macos-14-arm64. We have a very fragile list of checks. Presumably, CMake does a lot better, so use CMAKE_CXX_BYTE_ORDER in ByteOrderDefine (via config.h)

fix white-space

ead279e

run clang-format on Ubuntu on ByteOrderDefine.h. Apparently, there was a version mismatch.

NikEfth added 2 commits May 18, 2024 09:36

Fix single TOF position

e1149e2

Print TOF

14f3cea

KrisThielemans mentioned this pull request May 18, 2024

TOF mashing = TOF bins fixes #1434

Closed

KrisThielemans added 2 commits May 18, 2024 17:13

extra documentation for ML_estimate_component_based_normalisation

2100f42

[SWIG] expose ML_estimate etc.

a5d816a

KrisThielemans and others added 5 commits May 18, 2024 21:17

remove hard-wired 50 threads

a22c45c

use it as max

Some restructuring, needed.

4aa5466

Merge remote-tracking branch 'refs/remotes/origin/modifications_for_p…

b1333e7

…smr2024' into modifications_for_psmr2024

fix writing of single-TOF bin data

53c9d05

another fix on tof data

0c18fb6

KrisThielemans force-pushed the modifications_for_psmr2024 branch from c6337df to 0c18fb6 Compare May 19, 2024 13:17

KrisThielemans reviewed May 27, 2024

View reviewed changes

KrisThielemans added 2 commits June 1, 2024 18:17

Merge remote-tracking branch 'origin/master' into modifications_for_p…

9520af8

…smr2024

fix Interfile writing of TOF data with 1 TOF bin

7eea917

We need to preserve the TOF character, such that coincidence window etc is taken into account.

KrisThielemans requested changes Jun 1, 2024

View reviewed changes

replace std::cout calls with info()

d26b68a

KrisThielemans force-pushed the modifications_for_psmr2024 branch from ac803dd to d26b68a Compare June 1, 2024 19:13

KrisThielemans mentioned this pull request Jun 9, 2024

More openMP: find_min/max() and sum() #1449

Merged

Merge remote-tracking branch 'origin/master' into modifications_for_p…

34a35de

…smr2024

KrisThielemans mentioned this pull request Jul 24, 2024

Scanner coincidence window depends on TOF data #1481

Open

KrisThielemans mentioned this pull request Sep 6, 2024

MLEstimateComponentBasedNormalisation refactor #1499

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications for psmr2024 #1430

Modifications for psmr2024 #1430

NikEfth commented May 15, 2024

NikEfth commented May 15, 2024

KrisThielemans May 15, 2024

KrisThielemans May 17, 2024

KrisThielemans May 15, 2024

NikEfth May 16, 2024

KrisThielemans May 16, 2024

KrisThielemans commented May 16, 2024

NikEfth commented May 16, 2024

KrisThielemans left a comment

KrisThielemans May 16, 2024

KrisThielemans May 16, 2024

NikEfth commented May 16, 2024

KrisThielemans commented May 18, 2024

KrisThielemans commented May 18, 2024

KrisThielemans May 27, 2024

KrisThielemans May 27, 2024

KrisThielemans May 27, 2024

KrisThielemans May 27, 2024

KrisThielemans Jun 1, 2024

KrisThielemans Jun 1, 2024

KrisThielemans commented Jun 1, 2024 •

edited

Loading

KrisThielemans commented Jun 1, 2024

KrisThielemans commented Jun 9, 2024

KrisThielemans commented Jun 12, 2024

	if (min_timing_pos_num < 0 \|\| max_timing_pos_num > 1)
	if (min_timing_pos_num < 0 \|\| max_timing_pos_num > 0)

Modifications for psmr2024 #1430

Are you sure you want to change the base?

Modifications for psmr2024 #1430

Conversation

NikEfth commented May 15, 2024

Changes in this pull request

Testing performed

Related issues

Checklist before requesting a review

NikEfth commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented May 16, 2024

NikEfth commented May 16, 2024

KrisThielemans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NikEfth commented May 16, 2024

KrisThielemans commented May 18, 2024

KrisThielemans commented May 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrisThielemans commented Jun 1, 2024 • edited Loading

KrisThielemans commented Jun 1, 2024

KrisThielemans commented Jun 9, 2024

KrisThielemans commented Jun 12, 2024

KrisThielemans commented Jun 1, 2024 •

edited

Loading