Tangential position range consistency#1508
Tangential position range consistency#1508SomayehSaghamanesh wants to merge 2 commits intoUCL:masterfrom
Conversation
| const int min_tang_pos_num = -(num_detectors / 2); | ||
| const int max_tang_pos_num = -(num_detectors / 2) + num_detectors - 1; |
There was a problem hiding this comment.
I'm not sure about this. We have
STIR/src/buildblock/ProjDataInfoCylindricalNoArcCorr.cxx
Lines 206 to 208 in 0e667f7
Subtracting
diff = ((tp_num >> 1) + ((tp_num + 1) >> 1) + num_detectors/2 ) % num_detectors
Checking some examples (and relying on
STIR/src/buildblock/ProjDataInfoCylindricalNoArcCorr.cxx
Lines 168 to 169 in 0e667f7
diff = (tp_num + num_detectors/2 ) % num_detectors
Therefore, setting tp_num = -(num_detectors/2) gives diff==0. On the other hand, using the current max_tang_pos_num also gives us diff==0. Both are "self-coincidences", which don't make a lot of sense.
It is probably ok to have a too large look-up table (although see this comment, but I think that if you're having this problem, your num_tangential_poss is too large. The max is num_detectors -1 really (which arguably we should check).
KrisThielemans
left a comment
There was a problem hiding this comment.
I'm reluctant to merge this. If anything, we should keep the current max. However, this goes rather deep, and as you say 8 tests failed, I cannot see how we can merge as-is to cope with your case where num_tangential_poss=num_detectors_per_ring (which should mean that the first bin is always zero)
|
That's quite a lot of work to resolve this inconsistency. When I trim
projdata (which is feasible with trim_projdata), then I can safely work
with the current version of max, so I am OK if you skip these changes
(merge) and as you said, it fails many tests. So it would be great to only
keep those two utilities in the #1563 and skip this merge.
…On Wed, Nov 12, 2025 at 12:06 PM Kris Thielemans ***@***.***> wrote:
***@***.**** commented on this pull request.
I'm reluctant to merge this. If anything, we should keep the current max.
However, this goes rather deep, and as you say 8 tests failed, I cannot see
how we can merge as-is to cope with your case where
num_tangential_poss=num_detectors_per_ring (which should mean that the
first bin is always zero)
—
Reply to this email directly, view it on GitHub
<#1508 (review)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXA62ELPYDHNLG3LOJ35ZST34MIETAVCNFSM6AAAAACL32VWKCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTINJSHA4TGNZSGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
ok, I'm going to close this. Further comments on the trimming in #1563 |
Changes in this pull request
Changed min/max_tang_pos_num definitions in two classes to be consistent with ProjDataInfo.cxx.
Testing performed
It is built successfully but fails in 8 tests (in make test). Probably needs more modification?
I could handle projection files with tang180 (with lm_to_projdata and convert_projdata_types) without error. Before implementing above changes, I got below error:
"ERROR: The tangential_pos range (-90 to 89) for this projection data is too large. Maximum supported range is from -89 to 90"
Related issues
#1507
Checklist before requesting a review
documentation/release_XXX.mdhas been updated with any functionality change (if applicable)