-
Notifications
You must be signed in to change notification settings - Fork 90
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
Allow even number of ToF bins #1334
base: master
Are you sure you want to change the base?
Conversation
…et_k and swap s sym
@danieldeidda |
something is happening with the segmentIndices::_timing_pos whcich occasionally is different. Need to go deeper in debugging though |
else | ||
return new SymmetryOperation_PET_CartesianGrid_swap_zq(view180, axial_pos_shift, z_shift, transform_z); // s > 0 | ||
} | ||
else // segment_num = 0 | ||
{ | ||
if ( do_symmetry_swap_s && s < 0) return new SymmetryOperation_PET_CartesianGrid_swap_xmx_ymy(view180, axial_pos_shift, z_shift); | ||
if ( do_symmetry_swap_s && s < 0) return new SymmetryOperation_PET_CartesianGrid_swap_xmx_ymy(view180, axial_pos_shift, z_shift, proj_data_info_ptr->get_scanner_ptr()->get_max_num_timing_poss()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the problem is happening in one of these modifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually not sure anymore because the test is giving errors when the 2 bins are actually the same. also it happens for a non-tof test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you paste some sample errors, for both TOF and non-TOF?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WARNING: FactoryRegistry:: overwriting previous value of key in registry.
key: None
Tests for DataSymmetriesForBins_PET_CartesianGrid
Testing span=1
INFO: Detected view-mashing factor 24 from the number of views (8) and the number of detectors per ring (384).
Adjusting the azimuthal angle offset accordingly (an extra offset of 10.7813 degrees)
Tests with usual image size
INFO: Determined voxel size by dividing default_bin_size (3.12932) by zoom
Testing with all symmetries
WARNING: Disabling symmetries as image is rotated due to phi offset of the scanner.
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 0, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 1, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 2, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 3, view = 0, tangential_pos_num = -3, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -6, timing position index = 0
Symm bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -6, timing position index = 0
Error. Comparing symmetry bin configuration
Current bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -3, timing position index = 0
Symm bin: segment = -5, axial pos 4, view = 0, tangential_pos_num = -3, timing position index = 0
test failure in
Other debug asserts: |
src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx
Outdated
Show resolved
Hide resolved
this->max_num_central_timing_poss_to_process = | ||
this->proj_data_sptr->get_max_tof_pos_num(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think is wrong, it probably needs a divide by 2 or so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mh not sure than the shift is different then zero if we want to use them all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in fact, we don't support this yet, but having wrong code is a bad idea. Add a comment and throw an error if the argument isn't what we expect?
However, it seems hard to know what the argument should be. For 2 TOF bins, the max number of central should be 1. For 3 TOF bins, it should also be 1.5... (1 could work, but how would one say then we only want to do the central one). Looks like the name is still wrong.
Or remove...
src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx
Outdated
Show resolved
Hide resolved
I've merged
|
@danieldeidda I'd really like to release STIR 6.0.0 soon. We can easily postpone this to 6.1. Let me know. |
Hi I have been looking at it but haven't been able to find the source of the error. Let's postpone to 6.1 then |
src/include/stir/recon_buildblock/DataSymmetriesForBins_PET_CartesianGrid.inl
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/DataSymmetriesForBins_PET_CartesianGrid.inl
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/DataSymmetriesForBins_PET_CartesianGrid.inl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to have fixed the problem. Let's see how the test goes
src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx
Show resolved
Hide resolved
We will still have to think about consequences for IO. In particular, Siemens TOF order needed to be handled, so this code will have to be adapted STIR/src/IO/stir_ecat_common.cxx Lines 208 to 212 in 8ced2d7
|
Should put a check in STIR/src/buildblock/ProjDataFromStream.cxx Lines 152 to 155 in 8ced2d7
that values in the seq array are between min and max , to catch people using the "old" convention. (projdatainfo)
|
#1389 introduced the capability to read STIR/src/IO/InterfileHeader.cxx Line 621 in 8ced2d7
(Note that #1389 was after the 6.0.0 release, so we are not breaking backwards compatibility w.r.t. a released version). @NicoleJurjew I don't think this will affect you (as it's independent of the parsing of the Siemens Interfile header), but let us know if it does. |
change IO timing pos; add check in ProjDatafromStream; add glossary for TOF number
@@ -534,12 +534,14 @@ ProjDataInfoCylindrical::get_LOR(LORInAxialAndNoArcCorrSinogramCoordinates<float | |||
const float z1 = (m_in_mm - max_a * tantheta); | |||
const float z2 = (m_in_mm - min_a * tantheta); | |||
|
|||
const bool swap = false; // get_tof_delta_time(bin)<0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, this should be deleted, and the whole change w.r.t. master
reverted
we got through |
Co-authored-by: Kris Thielemans <KrisThielemans@users.noreply.github.com>
fixing #1333