Simple test for get_m() and scanner z_shift()

[NikEfth]

Simple quick test for get_m

[codacy-production]

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric	Results
Complexity	0
Duplication	0

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.}

[Copilot]

Pull request overview

Adds a focused regression test to validate ProjDataInfo::get_m() consistency across multiple Scanner geometry implementations, helping ensure axial-midpoint (m) semantics remain aligned when using Cylindrical, BlocksOnCylindrical, and Generic detector-coordinate maps.

Changes:

• Extend set_blocks_projdata_info() with an optional bin_fraction parameter to create lower-resolution projdata info for the new test.
• Add run_get_m_test() to test_proj_data_info.cxx to compare get_m() against LOR axial midpoints and across scanner models.
• Add a friend declaration in Scanner to allow the new test to call Scanner::set_detector_map().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File	Description
src/test/test_proj_data_info.cxx	Adds get_m() cross-geometry test and updates projdata-info construction helper to support bin downsampling.
src/include/stir/Scanner.h	Grants the new test class access to Scanner::set_detector_map() (currently private).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[KrisThielemans]

@NikEfth could we get this across the line? We're piling up PRs :-)

[NikEfth]

Done :)

[NikEfth]

I think this is done

[NikEfth]

I am sorry for the typos and mistakes this afternoon. Now it should be fine.

[KrisThielemans]

I guess we can merge once we rename get_det_pos_to_coord_type(). However, having a closer look at this, I think the difficulty really sits in Scanner. This has

STIR/src/include/stir/Scanner.h

Lines 631 to 633 in 45c403c

	shared_ptr<DetectorCoordinateMap> detector_map_sptr; /*! effective detection positions including average DOI */
	void set_detector_map(const DetectorCoordinateMap::det_pos_to_coord_type& coord_map);

I think the cleanest way to resolve this is to add get_detector_coordinate_map() const and set_detector_coordinate_map() (where the current set_detector_map() would then call set_detector_coordinate_map()) to Scanner. I'd even deprecate the current Scanner::set_detector_map, as it uses some internal stuff.

If we do that, we wouldn't need the mod to DetectorCoordinateMap.

Up to you...

[NikEfth]

I was not 100% if the setters should be deep-copy of the DetectorCoordinateMap. But in the previous implementation a new object was created ... so I kept that.

[KrisThielemans]

@NikEfth I've made some changes and a tiny clean-up (also of some unrelated things that I spotted).

[KrisThielemans]

segfault in CI for MacOS-conda-latest-compiler

35/74 Test #35: test_interpolate_projdata .......................................................***Exception: SegFault  1.32 sec

INFO: Performing symmetric interpolation test for BlocksOnCylindrical scanner

INFO: Determined voxel size by dividing default_bin_size (2) by zoom

INFO: Disabling all symmetries for the projector except for symmetry_z since they are not implemented in block geometry yet.

WARNING: ProjMatrixByBinUsingRayTracing used for pixel size (x,y)=(2,2) that is smaller than the central bin size (7.747817) divided by num_tangential_LORs (1).
This matrix will completely miss some voxels for some (or all) views. It is therefore to best to increase 'number of rays in tangential direction to trace for each bin'.

Second run crashed later in "asymmetric interpolation test for BlocksOnCylindrical scanner".

This seems unrelated to this PR, but it didn't occur in other Actions.

[KrisThielemans]

I'm running test_interpolate_projdata through valgrind on my Ubuntu 24.04. No problems. The above segfault could therefore be race condition, hopefully only appearing in MacOS clang.

[KrisThielemans]

@NikEfth this is ready now as far as I'm concerned. I intend to squash most commits before merging (today).

[KrisThielemans]

I've re-run CI 3 times now without any trouble. I therefore hope this was due to some temporary problems on the runner.

I'll go ahead and merge.

[NikEfth]

TBH, I think there is a race codition somewhere. I have seen similar crashes from time to time, especially in my mac. I don't think that it appears on this branch