test_blocks_on_cylindrical_projectors fails when compiled in debug

[robbietuk]

Relevant log

...
6: WARNING: Expected the number of views (22) to be related to the number of detectors per ring (180), but this is not the case. Continuing anyway (but without adjusting the azimuthal angle offset).
6: test_blocks_on_cylindrical_projectors: /home/rts/devel/stir/PrescientSTIR/STIR/src/include/stir/ProjDataInfoCylindrical.inl:214: int stir::ProjDataInfoCylindrical::get_view_mashing_factor() const: Assertion `get_scanner_ptr()->get_num_detectors_per_ring() % (2*get_num_views()) == 0' failed.
Exception:Subprocess aborted


0% tests passed, 1 tests failed out of 1

Total Test time (real) =   7.34 sec

The following tests FAILED:
	  6 - test_blocks_on_cylindrical_projectors (Subprocess aborted)
Errors while running CTest
Output from these tests are in: /home/rts/devel/stir/PrescientSTIR/STIR/cmake-build-debug/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.

Error summary

The assertion get_scanner_ptr()->get_num_detectors_per_ring() % (2*get_num_views()) == 0 is false. The issue starts here in the test:

STIR/src/recon_test/test_blocks_on_cylindrical_projectors.cxx

Lines 131 to 133 in 2eaacd6

	auto proj_data_info_blocks_sptr = std::make_shared<TProjDataInfo>(
	scanner_sptr, num_axial_pos_per_segment, min_ring_diff_v, max_ring_diff_v,
	scanner_sptr->get_max_num_views() / view_fraction, scanner_sptr->get_max_num_non_arccorrected_bins() / bin_fraction);

when using a SAFIRDualRingPrototype scanner. Then

STIR/src/buildblock/ProjDataInfoGenericNoArcCorr.cxx

Line 85 in 2eaacd6

find_cartesian_coordinates_of_detection(b1,b2,bin);

is called in the ProjDataInfoGenericNoArcCorr constructor, which leads to the error trace. I was able to follow this by adding a breakpoint on at the erring assert on line 214

STIR/src/include/stir/ProjDataInfoCylindrical.inl

Lines 207 to 217 in 2eaacd6

	int
	ProjDataInfoCylindrical::
	get_view_mashing_factor() const
	{
	// KT 10/05/2002 new assert
	assert(get_scanner_ptr()->get_num_detectors_per_ring() > 0);
	// KT 10/05/2002 moved assert here from constructor
	assert(get_scanner_ptr()->get_num_detectors_per_ring() % (2*get_num_views()) == 0);
	// KT 28/11/2001 do not pre-store anymore as set_num_views would invalidate it
	return get_scanner_ptr()->get_num_detectors_per_ring()/2 / get_num_views();
	}

The assert is needed because get_view_mashing_factor() returns an int type

[robbietuk]

This may be a local issue as a debug configuration is tested: https://github.com/UCL/STIR/actions/runs/7530331512/job/20496461728

[KrisThielemans]

a debug configuration is tested

yes, but the test is excluded:

STIR/.github/workflows/build-test.yml

Lines 316 to 318 in 434837c

	if test ${BUILD_TYPE} = Debug; then
	EXCLUDE="-E test_data_processor_projectors|test_export_array|test_ArcCorrection|test_blocks_on_cylindrical_projectors"
	fi

[KrisThielemans]

There's a few issues here:

1. the code uses mashing_factor=4 (as that's the default), which isn't appropriate for this scanner. However, due to Siemens choices (the Vision 600 uses 50 views, so a non-integer mashing factor), we cannot throw an error in the ProjData*NoArcCorr constructor. (Side note: of course, we don't use the Vision 600 yet with Blocks, and it would likely make no sense anyway for sinograms, due to the weird mashing they use).
   Setting view-mashing=1 would make the test pass
2. The only reason the constructor throws up this error (in Debug mode) is because it calls find_cartesian_coordinates_of_detection to set the z_shift (which we still need due to remove obsolete functions in ProjDataInfoCylindrical and ProjDataInfoCylindricalNoArcCorr #105). Arguably calling that function is overkill though, as we could just as well directly set this->z_shift=get_scanner_ptr()->get_coordinate_for_det_pos(DetectionPosition<>(0,0,0)).z() which would be clearer (see here)
   Fix is easy (but a bit scarier), and would prevent erroring in the constructor. However, it turns out that the test then throws a similar assert when calling get_sampling_in_s, as the Blocks stuff always assumes there is no compression anyway.
3. The get_det_pos_for_bin() etc functions only make sense when there's a unique correspondence, i.e. uncompressed data. get_all_det_pos* functions need integer view-mashing. We have no checks in non-Debug mode for any of this though (only assert), essentially for speed.
   Ideally we fix the last item without incurring a (noticeable) run-time overhead. That's non-trivial though. Ideally, this is checked in initialise_uncompressed_view_tangpos_to_det1det2(), which also ideally is only called when we need it. However, currently, we call this in the constructor due to (potential?) data-races #1255.

So, proposal

1. change the test to use default view-mashing=1
2. add a check in the BlocksNoArcCorr constructor
3. keep the issue open, as it will hit us when merging the cylindrical and Blocks/Generic hierarchy.

@markus-jehl @danieldeidda what do you think?

[markus-jehl]

As discussed offline with @KrisThielemans and @danieldeidda, I have first added a check for (or better: against) view mashing to the constructor of GenericNoArcCorr (couldn't do it in Blocks because that called the Generic constructor first and that already failed). This worked. Subsequently I then removed the view mashing in the tests - they take significantly longer now, though.

Additionally, I noticed that the TOF merge reverted the default muting of the following log message in the forward projector, which I took the liberty to bump up to verbosity 3 again: "INFO: Processing view x of segment y".