-
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
MLEstimateComponentBasedNormalisation refactor #1499
base: master
Are you sure you want to change the base?
MLEstimateComponentBasedNormalisation refactor #1499
Conversation
Thus far, I have not (intentionally) changed any of the functionality of the The only new introduction is |
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 comments. I am not 100% sure about updating to using shared_ptr
s but the idea is that the data can be accessed without copying.
Additionally, I am not convinced there is any testing of this code.
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
Outdated
Show resolved
Hide resolved
src/recon_buildblock/ML_estimate_component_based_normalisation.cxx
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
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.
some initial comments. I didn't review everything.
I'm not sure if we should provide access to the efficiencies_sptr
etc. shared_ptr
are nice, but to be avoided when we can. If we have a const auto get_efficiencies()
member, there seems tobe no point in making efficiencies
etc into a shared_ptr
.
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
Outdated
Show resolved
Hide resolved
src/recon_buildblock/ML_estimate_component_based_normalisation.cxx
Outdated
Show resolved
Hide resolved
src/recon_buildblock/ML_estimate_component_based_normalisation.cxx
Outdated
Show resolved
Hide resolved
src/recon_buildblock/ML_estimate_component_based_normalisation.cxx
Outdated
Show resolved
Hide resolved
src/include/stir/recon_buildblock/ML_estimate_component_based_normalisation.h
Outdated
Show resolved
Hide resolved
there are tests in the |
At this point, I want to add an interface with STIR/src/include/stir/recon_buildblock/BinNormalisationPETFromComponents.h Lines 60 to 62 in 33a0106
apply_normfactors3D uses streams to set these valuesSTIR/src/utilities/apply_normfactors3D.cxx Lines 71 to 83 in 33a0106
|
const ProjData& measured_projdata_v, | ||
const ProjData& model_projdata_v, |
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.
Should there be a check that these ProjData are the same "shape". Are there any tests for that?
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.
They should be. Not sure if it's checked anywhere.
Adds has_processed_data to MLEstimateComponentBasedNormalisation
I was just running out of time... Note that On another note, the "block" stuff really doesn't work and shouldn't be used. It was intended to cope with timing-alignment factors (for many scanners these are per block) but it introduces too much freedom (which I didn't know when I wrote that code). Timing alignment should be parametrised by 1 factor per block, not a norm-factor for every block-pair. Just saying that such that you don't spend too much time on that (i.e. maybe don't expose it) |
void | ||
BinNormalisationPETFromComponents::allocate(shared_ptr<const ProjDataInfo> pdi_sptr, MLEstimateComponentBasedNormalisation& normalization_estimator) | ||
{ | ||
if (!normalization_estimator.has_processed_data()) | ||
{ | ||
error("BinNormalisationPETFromComponents: internal error: allocate called on MLEstimateComponentBasedNormalisation " | ||
"without it having processed the data"); | ||
} | ||
this->proj_data_info_sptr = pdi_sptr; | ||
this->efficiencies = *normalization_estimator.get_efficiencies(); | ||
this->geo_data = *normalization_estimator.get_geo_data(); | ||
this->block_data = *normalization_estimator.get_block_data(); | ||
this->_already_allocated = true; | ||
} |
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 believe this is the cleanest way to implement all these changes. Here is a use case from my cpp program.
MLEstimateComponentBasedNormalisation ml_estimator(out_filename_tmp, *measured_projdata, *model_activity_projdata,
num_eff_iterations, num_iterations, do_geo, do_block,
do_symmetry_per_block, do_KL, do_display, do_save_to_file);
ml_estimator.process();
// Apply the normalization factors
BinNormalisationPETFromComponents bin_normalisation;
bin_normalisation.allocate(measured_projdata->get_proj_data_info_sptr(), ml_estimator);
bin_normalisation.set_up(measured_projdata->get_exam_info_sptr(), measured_projdata->get_proj_data_info_sptr());
// Compute the projdata
ProjDataInMemory normalization_projdata(*measured_projdata);
normalization_projdata.fill(1.F);
bin_normalisation.undo(normalization_projdata);
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 acknowledge #1498 (comment), however this does work and provides an alternative allocation of the data.
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'd rather do it the other way around I'm afraid. Your way is very different from anything else in STIR.
I very much prefer to let MLEstimateComponentBasedNormalisation
have a shared_ptr<BinNormalisationPETFromComponents> normalisation_sptr
member, which we can get via get_output()
(returning a shared_ptr<const BinNorm...>
). The MLEstimateComponentBasedNormalisation::get_efficiencies()
would then just forward to normalisation_sptr->get_efficiencies()
(note that I wouldn't let those return shared_ptr
then, but references). Any reason not to do it this way?
Infinities tend to come from 1/0. A candidate would be the |
Note that you'll have to rename class and file to |
This is failing because of a divide by 0. This because the projdata is 192 tangential positions but the fan size is calculated as 191 by const int half_fan_size = min(proj_data_info.get_max_tangential_pos_num(), -proj_data_info.get_min_tangential_pos_num());
fan_size = 2 * half_fan_size + 1; in here. There seems to be some requirement for the fan size to be odd? Regardless, during STIR/src/buildblock/ML_norm.cxx Lines 1167 to 1244 in e168b07
The issue is in the last for loop and the |
yes, that is the case at the moment. Code could be enhanced, but I was too lazy (having 1% extra data won't make or break the efficiency estimation) |
yes, so the TEMP FIX could/should handle this (the model has to be zero in that bin). Or you could trim the proj-data first (with SSRB) |
The The issue is when the I am not a huge fan of trimming the projdata with SSRB. I am trying to understand the code to figure out why the |
It's not the efficiencies themselves that are the problem, but the STIR/src/buildblock/ML_norm.cxx Lines 745 to 746 in e168b07
Moreover, there's plenty of loops doing
This isn't going to be so easy to fix. |
Note that #1430 updates the |
I am not up to date with the theory in the papers. Do you expect any issue if I fix the fan data by removing the |
This allows the test to pass and ignores the to ignore even num_tangential_poss issue
I changed the scanner to "Discovery 690", with an odd number of tangential positions ( The previous issue with |
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 check before I believe this is done.
@@ -73,7 +73,7 @@ main(int argc, char** argv) | |||
char* in_filename = new char[in_filename_prefix.size() + 30]; | |||
sprintf(in_filename, "%s_%s_%d_%d.out", in_filename_prefix.c_str(), "eff", iter_num, eff_iter_num); | |||
std::ifstream in(in_filename); | |||
in >> norm.crystal_efficiencies(); | |||
in >> norm.get_crystal_efficiencies(); |
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.
Returns by reference. This allows backwards compatibility with this code snippet but in general get_crystal_efficiencies()
method returning the efficiencies
object by reference might be dangerous?
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.
const &
would do the trick
src/include/stir/recon_buildblock/MLEstimateComponentBasedNormalisation.h
Show resolved
Hide resolved
src/include/stir/recon_buildblock/MLEstimateComponentBasedNormalisation.h
Outdated
Show resolved
Hide resolved
Pretty sure this PR is ready for review. I can't find any more outstanding issues. #1511 covers the other issue with the fan data and even numbers. Ill add to the release notes. |
…based_normalisation_redesign
This should be merged into v6.3. I have been using a branch derived from here for a while with no major issues. |
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.
Small changes required, but unfortunately quite a lot of them.
We cannot rename functions/files in 6.3, so I've flagged those up. Deprecations need to be listed in the release notes.
efficiencies = eff; | ||
} | ||
|
||
DetectorEfficiencies& get_crystal_efficiencies() |
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.
ok for changing to get/set
pair, but then I think we should have
DetectorEfficiencies& get_crystal_efficiencies() | |
const DetectorEfficiencies& get_crystal_efficiencies() const |
or would this break something?
However, cannot change backwards compatibility in a minor version update, so keep
#if STIR_VERSION < 070000
STIR_DEPRECATED DetectorEfficiencies& crystal_efficiencies()
...
#endif
Obviously the same for the others.
geo_data = geo; | ||
} | ||
|
||
GeoData3D& get_geometric_factors() |
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.
also const
block_data = block; | ||
} | ||
|
||
BlockData3D& get_block_factors() |
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.
also const
/*! | ||
\file | ||
\ingroup recon_buildblock | ||
\brief Declaration of stir::ML_estimate_component_based_normalisation |
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.
\brief Declaration of stir::ML_estimate_component_based_normalisation | |
\brief Declaration of stir::ML_estimate_component_based_normalisation and | |
stir::MLEstimateComponentBasedNormalisation |
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.
obviously, need to adjust when keeping ML_estimate_component_based_normalisation
in its original file.
\param do_display_v Whether to display the progress of the algorithm. | ||
\param do_save_to_file_v Whether to save the each iteration of the efficiencies, geo data and block data to file. | ||
*/ | ||
MLEstimateComponentBasedNormalisation(std::string out_filename_prefix_v, |
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.
generally STIR uses const std::string&
\ingroup recon_test | ||
\brief Test program to ensure MLEstimateComponentBasedNormalisation works as expected and | ||
produces the correct normalization factors from components. | ||
This is a very basic test that ensure the basic functionality. |
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 is a very basic test that ensure the basic functionality. | |
This is a test on basic functionality only. |
const auto scanner = std::make_shared<Scanner>(*Scanner::get_scanner_from_name("Discovery 690")); | ||
const auto exam_info = std::make_shared<ExamInfo>(ImagingModality::PT); |
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.
rename to scanner_sptr
and exam_info_sptr
const auto exam_info = std::make_shared<ExamInfo>(ImagingModality::PT); | ||
exam_info->patient_position = PatientPosition(PatientPosition::HFS); | ||
|
||
const shared_ptr<ProjDataInfo> projdata_info(ProjDataInfo::ProjDataInfoCTI( |
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.
use ProjDataInfo::construct_proj_data_info
(same signature)
/*views*/ scanner->get_num_detectors_per_ring() / 2, | ||
/*tang_pos*/ scanner->get_default_num_arccorrected_bins(), | ||
/*arc_corrected*/ false, | ||
/*tof_mash_factor*/ -1)); |
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.
why -1? Set to 0 for non-TOF.
const auto model_projdata = std::make_shared<ProjDataInMemory>(*measured_projdata); | ||
model_projdata->fill(2.F); | ||
|
||
constexpr int num_eff_iterations = 6; |
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 we make this smaller (to run faster)?
Also, you'll need to run |
Changes in this pull request
WIP
Refactor of
ML_estimate_component_based_normalisation
.Remaining tasks
get_efficiencies()
,get_norm_geo_data()
andget_norm_block_data()
do_geo
,do_block
,do_symmetry_per_block
,do_KL
,do_display
,do_save_to_file
efficiencies
,norm_geo_data
,norm_block_data
variables (even after all this work I am still slightly confused).Testing performed
Added a new unit test for basic functionality.
Related issues
Fixes #1498
Checklist before requesting a review
documentation/release_XXX.md
has been updated with any functionality change (if applicable)