reading of PETSIRD list-mode function by KrisThielemans · Pull Request #1604 · UCL/STIR

KrisThielemans · 2025-07-16T13:22:37Z

WIP

…its from classes

…D listmode data classes.

… information.

NikEfth

[x] there are some changes here for set_proj_data_info which probably came from another PR. Maybe they'll disappear when you merge master here

Merged with master is fine now.

[ x] there are changes here to cope with even number of TOF bins. They might be good, but it doesn't have anything to do with PETSIRD.

I know. But I had to make them as the data shared by people in the hackathon had even number of bins. If you you want to compare with that we need these changes here. I can document more.

[x] there's some changes in find_basic_vs_nums... which don't belong here

Yes that code doesn't do anything atm.

[x] I see no reason to make InputFormat::can_read non-const. If this is really required for PETSIRD, please explain. We should then work around it.

Done

[x] I guess the BinNormalisation isn't finished yet. I would suggest we currently call error and do it in a separate PR.

No it is finished. Unless you have comments.

NikEfth · 2026-03-20T10:22:04Z

+  - Mat3: std::array<std::array<float,3>,3> for compact fixed-size storage.
+  - Vec3: std::array<float,3> for simple 3D vectors.


OK. I started a bit of this. From the test_ . It won't be hard.

NikEfth · 2026-03-20T12:12:35Z

  else
    {
-      bin.timing_pos_num() = -timing_pos_num;
+      bin.timing_pos_num() = -timing_pos_num - (get_num_tof_poss() % 2 == 0);


I am sorry, I don't understand. We need to make adjustments for even and odd number of bins.

NikEfth · 2026-03-20T12:22:15Z

+  auto stir_scanner_sptr = petsird_info_sptr->get_scanner_sptr();
+
+  int tof_mash_factor = 1;
+  this->set_proj_data_info_sptr(std::const_pointer_cast<const ProjDataInfo>(


experimenting.

NikEfth · 2026-03-20T12:32:44Z

+      // for (int timing_pos_num = proj_data_info.get_min_tof_pos_num(); timing_pos_num <= proj_data_info.get_max_tof_pos_num();
+      //      ++timing_pos_num)


This is n't used atm.

KrisThielemans

some other clean-up suggestions

KrisThielemans · 2026-03-20T14:22:31Z

+find_unique_values(std::set<elemT>& values, const Array<num_dim, elemT>& input)
+{
+  for (int i = input.get_min_index(); i <= input.get_max_index(); ++i)
+    {
+      find_unique_values(values, input[i]);
+    }
+}


This will fail, as input[i] is an num_dim-1 array. You probably wanted to iterate using begin_all(), but as it doesn't create trouble, it means this function isn't used, so you could cut it. If it is needed, the 1D and nD implementation could be the same.

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

NikEfth

all done

NikEfth · 2026-03-20T17:22:41Z

Do you want the PETSIRDInfo_helpers with stir::Arrays?

codacy-production · 2026-05-22T09:48:48Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 219 complexity · 8 duplication

Metric Results

Complexity 219

Duplication 8

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

NikEfth · 2026-05-28T16:01:04Z

@casperdcl This is the PR

KrisThielemans · 2026-05-29T07:19:38Z


+template <typename elemT>
+inline void
+find_unique_values(std::set<elemT>& values, const Array<1, elemT>& input)


why not use use begin_all(), end_all(), then you need don' t need the template specialisation. In fact, I'm not sure if we then need the function at all as it's a 2 liner.

KrisThielemans · 2026-05-29T07:20:13Z

  else
    {
-      bin.timing_pos_num() = -timing_pos_num;
+      bin.timing_pos_num() = -timing_pos_num - (get_num_tof_poss() % 2 == 0);


this needs discussion and serious thought

KrisThielemans · 2026-05-29T07:24:16Z

+      string(REGEX MATCH "cxx_std_([0-9]+)" _petsird_std_match "${_petsird_features}")
+      if(CMAKE_MATCH_1)
+          UseCXX(${CMAKE_MATCH_1})
+      endif()


impressive enough, but should not be necessary AFAIK. If the target set C++ version, then it should automatically transcend. no?

Yes, that is true.

KrisThielemans · 2026-05-29T07:31:55Z

+#include "stir/ProjData.h"
+#include "stir/ProjDataInfo.h"
+#include "stir/listmode/CListRecord.h"
+#include "stir/IO/InputStreamWithRecords.h"


all of these are unused in the .h, so should be removed. However, we do need DetectorCoordinateMap.h

KrisThielemans · 2026-05-29T07:33:11Z

+  If listmode reconstruction is done, the map is regenerated on-the-fly.
+
+*/
+class CListModeDataPETSIRD : public CListModeDataBasedOnCoordinateMap


whenever you're ready, move implementations to .cxx

KrisThielemans · 2026-05-29T07:34:44Z

+#include <iostream>
+#include <fstream>
+#include "stir/Succeeded.h"


looks like we don't need any of this

KrisThielemans · 2026-05-29T07:43:26Z

+
+    SPDX-License-Identifier: Apache-2.0
+    See STIR/LICENSE.txt for detail
+/*!


close comment first (compiler warning otherwise)

KrisThielemans · 2026-05-29T07:46:49Z

+#include "stir/DetectionPosition.h"
+#include "stir/DetectionPositionPair.h"
+#include "petsird/protocols.h"
+#include "stir/Scanner.h"
+#include "stir/DetectorCoordinateMap.h"
+#include "stir/ProjDataInfo.h"
+#include <set>
+#include "stir/format.h"
+#include "stir/info.h"
+#include "stir/error.h"
+
+#include "petsird_helpers.h"
+#include "petsird_helpers/create.h"   // for make_detection_bin
+#include "petsird_helpers/geometry.h" // depending on where get_detection_efficiency lives


only include whatever is needed for the class definition and .inl (which should be as little as possible). Several reasons:

easier to understand

less auto-generated dependencies, so faster recompilation

no (less?) issues with compiling registries, like we currently have for the MacOS job
in file included from /Users/runner/work/STIR/STIR/src/recon_buildblock/recon_buildblock_registries.cxx:87: In file included from /Users/runner/work/STIR/STIR/src/include/stir/recon_buildblock/BinNormalisationFromPETSIRD.h:27: In file included from /Users/runner/work/STIR/STIR/src/include/stir/PETSIRDInfo.h:25: /Users/runner/work/STIR/STIR/src/include/stir/format.h:40:12: fatal error: 'fmt/format.h' file not found 40 | # include "fmt/format.h"

KrisThielemans

Looks like CI is working! Great. (The MacOS job fails because of too much stuff in .h/.inl, see previous review).

KrisThielemans · 2026-05-29T08:18:52Z

          case ${{matrix.os}} in
            (ubuntu* | macOS*)
-              sudo .github/workflows/GHA_increase_disk_space.sh
+              if [ "${ACT:-false}" = "true" ]; then


what is this? Is it https://github.com/nektos/act? Please add a comment line

I forgot to remove it. I ran to problems executing the CI locally, as I don't have Github's infracture and this stops the execution of the GHA_increase_disk_space.
I run the CI like this:
act -W .github/workflows/build-test.yml -P ubuntu-24.04=ghcr.io/catthehacker/ubuntu:act-24.04 --env ACT=true

keep it in. Seems quite useful anyway. But document, and add 2 lines in the developers guide

KrisThielemans · 2026-05-29T08:21:42Z

+        cache-environment: true
+        cache-downloads: true
+
+    - name: Add STIR dependencies in PETSIRD environent


Suggested change

- name: Add STIR dependencies in PETSIRD environent

- name: Add STIR dependencies in PETSIRD environment

KrisThielemans · 2026-05-29T08:23:23Z

+      shell: bash -el {0}
+      run: |
+        set -ex
+        micromamba install -y -n yardl -c conda-forge boost-cpp swig matplotlib


Hopefully we don't need matplotlib. It's a large dependency...

Also, I'm doubtful of having boost-cpp and swig here, but also via APT/brew below. Do we really need this here?

KrisThielemans · 2026-05-29T08:25:49Z

+            mkdir -p build 
+            cd build 
+
+            cmake .. \


Suggested change

cmake .. \

cmake .. \

-G Ninja \

as we use that for STIR itself, it provides clearer messages, and means we don't need make

KrisThielemans · 2026-05-29T08:27:10Z

          BUILD_TYPE: "Release"
          parallelproj: "ON"
-          ROOT: "ON"
+          ROOT: "OFF"


KrisThielemans and others added 13 commits July 14, 2025 17:54

add PETSIRD submodule

5a85ad6

* Initial commit that adds PETSIRD support in CMake configuration

561616e

WIP: Subclass CListModeDataSAFIR from CListModeDataBasedOnCoordinateMap.

d6c218a

Update .gitmodules

8ff962a

Remove PETSIRD submodule from .gitmodules

557cf97

Fix the include and link paths

493a0c0

* Minor fix

27ec9f4

creating classes for LM PETSIRD, mainly a copy at this stage

0e0b71b

move cmake importing of sird into lmbuildblock remove: unnecessary b…

6d00cde

…its from classes

correct cmake importing of sird into lmbuildblock. can import binary/

0f9deb2

started reading info from header

3550178

Work in separating the SAFIR from the BasedOnCoordinateMAP and PETSIR…

32daa97

…D listmode data classes.

Merge branch 'PETSIRD' of github.com:UCL/STIR into PETSIRD

b874c8e

KrisThielemans self-assigned this Jul 16, 2025

NikEfth and others added 16 commits July 16, 2025 11:03

Important fix in CListModeDataPETSIRD.inl

ba44a95

* Fixes and interface clean up

0ff78b1

Removed constructor.

f6de251

reading header and extracting scanner info

43febf1

Fixed registries! :)

12e6f57

Merge branch 'PETSIRD' of github.com:UCL/STIR into PETSIRD

828817e

Updates on the interface and CMake

4897222

Compiles and LInks

ddc06a5

Finished all the infrastrucure. We can now focus on reading events,

3a4ee34

reading header and extracting averag position of each detector

d7a00cc

Finalizing the Interface with PETSIRD.

32c3144

Trying to figure out how the scanner looks like.

4b39461

Create ProjDataInfoBlocksOnCylindricalNoArcCorr from PETSIRD

5499245

Merge remote-tracking branch 'origin/master' into PETSIRD

d029d0d

* Clean up the code that creates a Scanner object from PETSRID header…

d00e987

… information.

* Stop testing for PETSIRD

d0e46a4

NikEfth added 3 commits March 20, 2026 11:56

Housekeeping

9d7e6e5

Housekeeping

b2beaaa

More housekeeping

7953f4c

NikEfth reviewed Mar 20, 2026

View reviewed changes

NikEfth added 3 commits March 20, 2026 13:58

fix

f1ef0c7

fix

f6ec82e

Fix CODACY

784d484

NikEfth marked this pull request as ready for review March 20, 2026 13:04

Precommit

1a30680

KrisThielemans commented Mar 20, 2026

View reviewed changes

NikEfth and others added 6 commits March 20, 2026 17:18

Apply suggestion from @KrisThielemans

1b30940

Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>

house keeping.

6dfa5ad

Pre commit

9be1d46

Merge branch 'PETSIRD' of github.com:UCL/STIR into PETSIRD

e660768

Fix

c54394d

Deleted obsolete file.

a4812c5

NikEfth reviewed Mar 20, 2026

View reviewed changes

NikEfth added 3 commits March 20, 2026 19:45

cleaner bits.

3702d94

Merge remote-tracking branch 'origin/master' into PETSIRD

4a658c6

Mainly copyright info

e8e6287

NikEfth added 4 commits May 22, 2026 12:32

pre-commit

c24b9c9

CI with PETSRID (WIP)

54b31f8

Fixes.

3f3affb

Full CI.

ddcdebe

Replace nproc with parallel.

f4a90cd

KrisThielemans commented May 29, 2026

View reviewed changes

		- Mat3: std::array<std::array<float,3>,3> for compact fixed-size storage.
		- Vec3: std::array<float,3> for simple 3D vectors.

		// for (int timing_pos_num = proj_data_info.get_min_tof_pos_num(); timing_pos_num <= proj_data_info.get_max_tof_pos_num();
		// ++timing_pos_num)

	- name: Add STIR dependencies in PETSIRD environent
	- name: Add STIR dependencies in PETSIRD environment

Conversation

KrisThielemans commented Jul 16, 2025

Uh oh!

NikEfth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NikEfth left a comment

Choose a reason for hiding this comment

Uh oh!

NikEfth commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codacy-production Bot commented May 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Up to standards ✅

Uh oh!

NikEfth commented May 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrisThielemans May 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

NikEfth commented Mar 20, 2026 •

edited

Loading

codacy-production Bot commented May 22, 2026 •

edited

Loading

KrisThielemans May 29, 2026 •

edited

Loading