Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug in upsample_and_fit_scatter_estimate for cylindrical #1224

Closed
KrisThielemans opened this issue Jul 14, 2023 · 1 comment · Fixed by #1225
Closed

bug in upsample_and_fit_scatter_estimate for cylindrical #1224

KrisThielemans opened this issue Jul 14, 2023 · 1 comment · Fixed by #1225
Assignees
Labels
Milestone

Comments

@KrisThielemans
Copy link
Collaborator

KrisThielemans commented Jul 14, 2023

Current code reads

ProjDataInMemory interpolated_direct_scatter(emission_proj_data.get_exam_info_sptr(),
interpolated_direct_scatter_proj_data_info_sptr);
bool actual_remove_interleaving = remove_interleaving;
if (remove_interleaving && emission_proj_data.get_proj_data_info_sptr()->get_scanner_sptr()->get_scanner_geometry()!="Cylindrical")
{
warning("upsample_and_fit_scatter_estimate: forcing remove_interleaving to false as non-cylindrical projdata");
actual_remove_interleaving = false;
}
if (emission_proj_data.get_proj_data_info_sptr()->get_scanner_sptr()->get_scanner_geometry()=="Cylindrical")
interpolated_direct_scatter_proj_data_info_sptr->reduce_segment_range(0,0);

that seems incorrect, as the interpolated_direct_scatter_proj_data_info_sptr is reduced in size after creating interpolated_direct_scatter. This happened at
1b128f2 part of #1006. @danieldeidda we had a conversation in that PR about it, but in the end left it as-is.

Original code was

shared_ptr<ProjDataInfo>
interpolated_direct_scatter_proj_data_info_sptr(emission_proj_data.get_proj_data_info_sptr()->clone());
interpolated_direct_scatter_proj_data_info_sptr->reduce_segment_range(0,0);
info("upsample_and_fit_scatter_estimate: Interpolating scatter estimate to size of emission data");
ProjDataInMemory interpolated_direct_scatter(emission_proj_data.get_exam_info_sptr(),
interpolated_direct_scatter_proj_data_info_sptr);

Found while debugging the TOF branch with @NicoleJurjew

@KrisThielemans KrisThielemans added this to the v5.2 milestone Jul 14, 2023
@KrisThielemans KrisThielemans self-assigned this Jul 14, 2023
@KrisThielemans
Copy link
Collaborator Author

In fact, I think we should reduce the segment range to 0,0 also for blocks. Currently, we are calling interpolate_projdata (whcih handles segment 0 only), and inverse_SSRB in all cases. Therefore, allocating interpolated_direct_scatter with only the 0 segment is clearer and will save memory.

(TBH, it is not clear to me why @NicoleJurjew 's modification of putting a TOF-loop in interpolate_projdata caused a crash if we don't fix this issue, as it first sight it should just create a "too large" interpolated_direct_scatter, but still work I guess. In any case, the current code is inefficient).

KrisThielemans added a commit to KrisThielemans/STIR-1 that referenced this issue Jul 14, 2023
As interpolate_projdata only handles segment 0 at present, we only need
to create an appropriately sized direct_scatter projdata.

Also minor clean-up of this function (localise variable, remove unused
variable, extra comments)

Fixes UCL#1224
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant