-
Notifications
You must be signed in to change notification settings - Fork 95
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
Time Of Flight reconstruction #304
Conversation
@NikEfth, did you run the recon_test_pack locally? Travis throws up a lot of problems in it. For instance core dumps in the ROOT tests, but also others. it's always difficult to know with Travis of course. |
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.
a few house-keeping comments. nothing actually important.
src/include/stir/ProjDataInfo.h
Outdated
@@ -221,6 +221,9 @@ class ProjDataInfo | |||
inline int get_num_tangential_poss() const; | |||
//! Get number of tof bins | |||
inline int get_tof_bin(const double& delta) const; | |||
|
|||
inline int get_unmashed_tof_bin(const double& delta) const; |
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.
don't use references for float
, double
etc. No point and it could slow it down.
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.
add a doxygen comment
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.
🆗
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 don't think we should have this function. For some scanners this is not achievable.
@@ -0,0 +1,106 @@ | |||
#! /bin/sh |
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.
correct name of file
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.
🆗
src/CMakeLists.txt
Outdated
@@ -33,8 +33,6 @@ option(BUILD_EXECUTABLES | |||
option(BUILD_SHARED_LIBS | |||
"Use shared libraries" OFF) | |||
|
|||
### Settings for external libraries |
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.
somehow removed. re-instate I guess (I haven't checked properly)
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 was weird. I merged with master and still this line did not re-appear. Only when I checked out the file.
@@ -56,7 +56,7 @@ InputStreamFromROOTFileForCylindricalPET(std::string _filename, | |||
up_energy_window = _up_energy_window; | |||
offset_dets = _offset_dets; | |||
|
|||
half_block = module_repeater_y * submodule_repeater_y * crystal_repeater_y / 2 - 1; | |||
half_block = static_cast<int>( (module_repeater_y * submodule_repeater_y * crystal_repeater_y) / 2); |
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.
a bit weird to have a cast. The variables are all ints?
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 was getting a warning. But seems to be ok.
@@ -204,6 +207,13 @@ set_up(const std::string & header_path) | |||
if (nentries == 0) | |||
error("InputStreamFromROOTFileForCylindricalPET: The total number of entries in the ROOT file is zero. Abort."); | |||
|
|||
return Succeeded::yes; | |||
half_block = static_cast<int>( (module_repeater_y * submodule_repeater_y * crystal_repeater_y) / 2); |
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.
code never executed
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.
🆗
src/IO/interfile.cxx
Outdated
for (seg++; seg != segment_sequence.end(); seg++) | ||
output_header << "," <<proj_data_info_ptr->get_max_ring_difference(*seg); | ||
output_header << "}\n"; | ||
std::vector<int>::const_iterator seg = segment_sequence.begin(); |
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.
more white space
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.
How would you like me to address this? Revert the file and reapply the changes?
src/buildblock/ML_norm.cxx
Outdated
(*segment_ptr)[bin.axial_pos_num()][bin.view_num()][bin.tangential_pos_num()]; | ||
for (bin.timing_pos_num() = proj_data.get_min_tof_pos_num(); bin.timing_pos_num() <= proj_data.get_max_tof_pos_num(); ++ bin.timing_pos_num()) | ||
{ | ||
segment_ptr.reset(new SegmentBySinogram<float>(proj_data.get_segment_by_sinogram(bin.segment_num(),bin.timing_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.
this seems incorrect. fan-sums etc should sum over TOF bins. with current code. the fan data seems to be the one from the last TOF bin.
adaptations in ML_norm seems to need more thought. Can we just error out for now if its 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.
Sure. Done.
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 applied almost all comments.
The recon_test_past should not be able to function, yet.
One more commit for that.
@@ -0,0 +1,106 @@ | |||
#! /bin/sh |
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.
🆗
src/CMakeLists.txt
Outdated
@@ -33,8 +33,6 @@ option(BUILD_EXECUTABLES | |||
option(BUILD_SHARED_LIBS | |||
"Use shared libraries" OFF) | |||
|
|||
### Settings for external libraries |
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 was weird. I merged with master and still this line did not re-appear. Only when I checked out the file.
@@ -56,7 +56,7 @@ InputStreamFromROOTFileForCylindricalPET(std::string _filename, | |||
up_energy_window = _up_energy_window; | |||
offset_dets = _offset_dets; | |||
|
|||
half_block = module_repeater_y * submodule_repeater_y * crystal_repeater_y / 2 - 1; | |||
half_block = static_cast<int>( (module_repeater_y * submodule_repeater_y * crystal_repeater_y) / 2); |
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 was getting a warning. But seems to be ok.
src/include/stir/ProjDataInfo.h
Outdated
@@ -221,6 +221,9 @@ class ProjDataInfo | |||
inline int get_num_tangential_poss() const; | |||
//! Get number of tof bins | |||
inline int get_tof_bin(const double& delta) const; | |||
|
|||
inline int get_unmashed_tof_bin(const double& delta) const; |
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.
🆗
src/buildblock/ML_norm.cxx
Outdated
(*segment_ptr)[bin.axial_pos_num()][bin.view_num()][bin.tangential_pos_num()]; | ||
for (bin.timing_pos_num() = proj_data.get_min_tof_pos_num(); bin.timing_pos_num() <= proj_data.get_max_tof_pos_num(); ++ bin.timing_pos_num()) | ||
{ | ||
segment_ptr.reset(new SegmentBySinogram<float>(proj_data.get_segment_by_sinogram(bin.segment_num(),bin.timing_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.
Sure. Done.
src/IO/interfile.cxx
Outdated
for (seg++; seg != segment_sequence.end(); seg++) | ||
output_header << "," <<proj_data_info_ptr->get_max_ring_difference(*seg); | ||
output_header << "}\n"; | ||
std::vector<int>::const_iterator seg = segment_sequence.begin(); |
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.
How would you like me to address this? Revert the file and reapply the changes?
Fix a missing function declaration.
All tests pass. I have not checked the recon_tests yet
current Travis failures
The |
re-enabling some tests in test_proj_data_in_memory
|
@ALEXJAZZ008008 @NikEfth in my previous commit, I changed It's easily fixable, but the question is what This did show that there are currently no Python (or of course MATLAB) tests for the TOF code. Maybe @ALEXJAZZ008008 can contribute some... |
oh, maybe the Python test does check it, as I see my comment above that it's failing... Also, listmode recon... |
I think However, now this feels a bit counter-intuitive. Because by default the tof positions are the "outer" layer of our |
|
cannot test right now. Let's see what travis will say.
src/test/test_time_of_flight.cxx
Outdated
@@ -47,10 +47,10 @@ START_NAMESPACE_STIR | |||
//! | |||
class cache_index{ | |||
public: | |||
cache_index() { | |||
cache_index(): |
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 doesn't seem to change anything at all, but best to use constructor initialisers. I don't see where cache_index
is used by the way
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 did this for codacy.
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.
great. but is this class actually used?(sorry, can't see it)
src/swig/stir.i
Outdated
int num_sinos = proj_data.get_num_sinograms(); | ||
|
||
Array<4,float> array(IndexRange4D(proj_data.get_num_tof_poss(),num_sinos, proj_data.get_num_views(), proj_data.get_num_tangential_poss())); | ||
Array<4,float> array(IndexRange4D(proj_data.get_num_tof_poss(),proj_data.get_num_segments(), proj_data.get_num_views(), proj_data.get_num_tangential_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.
incorrect. This would have to be
const int num_non_tof_sinos = proj_data.get_num_sinograms() / proj_data.get_num_tof_poss();
and then use that. I'll fix this.
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.
t.b.h. then I do not like this.
We have to make sure that under no situation the get_num_for_poss()
does not return zero.
Which currently is the default value for the creation of a ProjDataInfo
....
The mashing factor is zero.
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've done this slightly differently (by introducing get_num_non_tof_sinograms
), but get_num_tof_poss()
should never be zero of course. I guess we'd need a test for this then.
I don't understand what you mean with "The mashing factor is zero" (but possibly it's no longer relevant)
I took these values from #11, but cannot check them.
TOF clean-up
…1_02 Merge master to TOF 2024 01 02
Add facility to add an alias for a keyword. This just works by looking-up the keyword in a map. This meansss type and callback remain the same. Also minor clean-up of test_KeyParser
use hopefully clearer names: - new: "TOF mashing factor", old: "%TOF mashing factor" - new: "Maximum number of (unmashed) TOF time bins", old: "Number of TOF time bins" - new: "Size of unmashed TOF time bins (ps)", old: "Size of timing bin (ps)" Old keywords are entered as aliases until STIR 7.0
changed TOF-related interfile keywords
Use get_bin_for_det_pos_pair instead. Addresses UCL#105
make get_bin_for_det_pair private and doc TOF argument
Fixes line width in python documentation, markdown bullet point spacing and Prettier formatting, and default nonTOF_distance_threshold in consistency test to 0.f
We were still writing "%TOF mashing factor".
also add changes to this function to release notes
was "Timing resolution (ps)"
also use new names for the keywords
this isn't currently used for parsing though
this is a work_around for UCL#1302
TOF interfile and scanner fixes
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 will have to do for now. Outstanding issues and GE data will have to be handled on master
.
Thanks to everyone!
*/ | ||
template <class coordT> | ||
inline void | ||
project_point_on_a_line( |
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.
@robbietuk could you check this? Maybe we can remove some duplication.
Introduces TOF reconstuction in STIR