-
Notifications
You must be signed in to change notification settings - Fork 29
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
TOF #543
TOF #543
Conversation
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.
Looks good to me! Hopefully you're getting better results now...
I think we'll need to do the same as in STIR: get_num_sinograms
is as it is now, but add get_num_non_TOF_sinograms
(in SIRF with capital, as we're using get_num_TOF_bins
). Will need some doxygen on the C++-side then to clarify. Will also need to be exposed to Python and MATLAB (with appropriate doc).
I found a few other places where the documentation will have to say something like number of (non-TOF) sinograms
. Search for TOF
in:
- src/xSTIR/mSTIR/+sirf/+STIR/AcquisitionData.m
- examples/Python/PET/input_output.py
- examples/Matlab/PET/acquisition_data_from_scanner_info.m
@evgueni-ovtchinnikov, could you help with that?
This isn't urgent of course, as we cannot merge this until STIR TOF has been merged.
Potential strategy: add dummy TOF functions (get_num_tof_poss
and get_num_non_tof_sinograms
) on STIR master, such that this could be merged already. Good idea?
will have to modify .travis.yml to use the TOF STIR branch I guess |
travis pulls stir tof branch updated docs to refelct change to get_num_sinograms
I am doing this on release_4. UCL/STIR#762 This should allow us to merge this SIRF PR |
…f that This update should make this PR with standard STIR and the TOF branch (once UCL/STIR#762 is merged)
UCL/STIR#762 is merged on STIR |
Happy to say that all builds succeeded, except when Of course, none of the builds actually test TOF functionality. |
pending UCL/STIR#764 |
This should be fine now but running one more check. @evgueni-ovtchinnikov do we need any (urgent) changes on the MATLAB side? |
@evgueni-ovtchinnikov I'll merge this. Please make any urgent changes for MATLAB separately. Otherwise, add them to #555 |
@KrisThielemans MATLAB scripts run ok except for |
@danieldeidda This will be due to UCL/STIR#672 I guess. this shouldn't have happened I believe |
Mh yes definetely because of UCL/STIR#672 |
I think the problem is that I check that calib_factor>0 but than set it to one for ECAT etc. |
@evgueni-ovtchinnikov could you confirm which commit you are on in STIR when this happens? We think that it shouldn't happen (on current |
@evgueni-ovtchinnikov also, can you give some more output to tell us when this happened |
commit fe1af133aee5c44ce9fecd50993612f47d274b44
|
WARNING: KeyParser: keyword 'isotope name' already registered for parsing, overwriting previous value WARNING: KeyParser: keyword '%comment' already registered for parsing, overwriting previous value Processing time frame 1 500000 events stored This took 4.34375s CPU time. This took 2.40625s CPU time. ERROR: ChainedBinNormalisation: both first and second have a calibration factor. The factor would be applied twice |
The issue has been fixed by UCL/STIR#800 |
partially addresses issues in #315 and #390