Skip to content

simplify ProjDataInfoGeneric hierarchy#1696

Open
KrisThielemans wants to merge 9 commits intoUCL:masterfrom
KrisThielemans:ProjDataInfoGenericHierarchySwap
Open

simplify ProjDataInfoGeneric hierarchy#1696
KrisThielemans wants to merge 9 commits intoUCL:masterfrom
KrisThielemans:ProjDataInfoGenericHierarchySwap

Conversation

@KrisThielemans
Copy link
Copy Markdown
Collaborator

@KrisThielemans KrisThielemans commented Mar 20, 2026

This PR implements the steps discussed in #1307 (comment)

@KrisThielemans KrisThielemans self-assigned this Mar 20, 2026
@KrisThielemans
merge ProjDataInfoGeneric into ProjDataInfoGenericNoArcCorr
ef474fd 
This is step 1 in UCL#1307 (comment)

ProjDataInfoGeneric has no good reason to exist on its own,
as it assumes discretised detectors already, as does
ProjDataInfoGenericNoArcCorr. Merging the 2 simplifies the
hierarchy.
@KrisThielemans KrisThielemans force-pushed the ProjDataInfoGenericHierarchySwap branch from 40d60c0 to a51fb74 Compare March 21, 2026 00:32
@KrisThielemans
Copy link
Copy Markdown
Collaborator Author

KrisThielemans commented Mar 21, 2026

Step 1 and 2 implemented.

Warning: I will squash some commits here once it works.

@KrisThielemans
derive ProjDataInfoGenericNoArcCorr from ProjDataInfoCylindricalNoArc…
b9f8b61 
…Corr

This is step 2 in UCL#1307 (comment)

By deriving from ProjDataInfoCylindricalNoArcCorr, we can cut most functions
in the Generic version, as they are duplicates.
@KrisThielemans KrisThielemans force-pushed the ProjDataInfoGenericHierarchySwap branch from d258b74 to b9f8b61 Compare March 21, 2026 01:28
@KrisThielemans
small simplification of ProjDataInfoBlocksOnCylindricalNoArcCorr
3081a37 
removed one function as it's now implemented higher up
(t is obsolete anyway)
@KrisThielemans
Copy link
Copy Markdown
Collaborator Author

KrisThielemans commented Mar 21, 2026

Steps 1 & 2 are done, although I suddenly see segfaults in the MacOSDebug build in CI. I hope they will disappear with the later updates as that will be hard to debug (I have no access to a Mac).

Note that step 3 is for v7, as at the moment the ProjDataInfoBlocksOnCylindricalNoArcCorr still has a useful function that it checks it is indeed a "blocks" scanner.

We could rename ProjDataInfoGenericNoArcCorr in this PR as the final step (with a using to preserve compatibility), as it's current name really makes no sense. Maybe we should leave that for v7 though.

I think we only need 2 small changes to enable TOF (currently I still call error() when it's TOF data in the generic case), but to do that, we would need new test cases. In the interest of merging, we should probably do that in a different PR.

@KrisThielemans
remove incorrect assert and replace BOOST_STATIC_ASSERT
750f708 
The assertt() call was in the wrong place and caused recursive
calls in the "generic" case.

I took the opporttunity to remove on more dependency on boost.
@KrisThielemans KrisThielemans marked this pull request as ready for review March 21, 2026 16:17
@KrisThielemans KrisThielemans added this to the v6.4 milestone Mar 21, 2026
@KrisThielemans
Copy link
Copy Markdown
Collaborator Author

KrisThielemans commented Mar 21, 2026

@markus-jehl could you try this out, please?

@KrisThielemans
updated release notes
cffbbee 
[ci skip]
@KrisThielemans
minor clean-up in ProjDataInfoGenericNoArcCorr::get_bin
9bc5ebf 
also prepare for TOF (but still disabled)
@KrisThielemans
ProjDataInfoGenericNoArcCorr: safer handling of "cylindrical" functions
e717d30 
We were using "delete", but that means they could be called inadvertently.
This was now more confusing with the change in hierarchy. Instead, we now
call error() in most cases.

Also updated TOF functionality, but it's still disabled.
@KrisThielemans
use stir::format() in warning()
203506a
@KrisThielemans
call error() when using azimuthal tilt for generic scanner
787345c 
we currently ignore this field and rely on the detector map,
so now check that it's zero.
@KrisThielemans KrisThielemans force-pushed the ProjDataInfoGenericHierarchySwap branch from 4c711da to 787345c Compare March 22, 2026 00:02
@KrisThielemans
Copy link
Copy Markdown
Collaborator Author

KrisThielemans commented Mar 22, 2026

The code is a bit safer now. (I started testing TOF and ran into some unexpected errors because of the "deleted" functions, so now we call error instead). I've committed that in this PR as it doesn't have anything to do with TOF really.

@KrisThielemans
Copy link
Copy Markdown
Collaborator Author

KrisThielemans commented Mar 22, 2026 •
edited
Loading

Enabling a block scanner in test_proj_data_info runs into trouble because ProjDataInfoGenericNoArcCorr::get_bin(lor) only handles LORAs2Points where the end points are directly in the detector coordinate map. This turns out not to be the case in test_generic_proj_data_info (where they are end points on a cylinder). However, coping with this would be very slow (it seems to need an expensive loop to see which of all the detector coordinates the LOR goes through...).

Of course, this problem is identical to what is currently on master, so it shouldn't stop us from merging. (It's a pity though, as it'd give us a bunch of TOF tests for free)

@KrisThielemans
Copy link
Copy Markdown
Collaborator Author

KrisThielemans commented Mar 28, 2026

@markus-jehl @NikEfth @danieldeidda I will merge this soon. It is working exactly as before, to the best of my knowledge.

Do test, even after the merge...

@markus-jehl
Copy link
Copy Markdown
Contributor

markus-jehl commented Mar 30, 2026 •
edited
Loading

@KrisThielemans sorry, only saw this now. Managed to get around testing it now: works fine for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NikEfth NikEfth Awaiting requested review from NikEfth

@danieldeidda danieldeidda Awaiting requested review from danieldeidda

Assignees

@KrisThielemans KrisThielemans

Labels

enhancement

Projects

None yet

Milestone

v6.4

Development

Successfully merging this pull request may close these issues.

2 participants

@KrisThielemans @markus-jehl