UCL / STIR

Software for Tomographic Image Reconstruction
http://stir.sourceforge.net/
Other
110 stars 91 forks source link

`ML_estimate_component_based_normalisation` and `apply_norm_factors3D` cannot be used without saving to disk #1498

Open robbietuk opened 3 weeks ago

robbietuk commented 3 weeks ago

These are executable files for computing normalization and a brief description of how they are implmented:

Most of the following pertains to find_ML_normfactors3D and find_ML_normfactors. The above functionality saves the computed data to be saved to disk, rather than returning/utilizing a shared_ptr.


The core functionality of these executables should be split into various components.

  1. The main function should handle only the arguments handling and saving data.
  2. Mostly, functions/classes that take the core arguments and compute/process the data.
  3. Can handle saving of the data, if specified by the arguments.
  4. Function/ object returns a pointer to the computed data, that can be saved by the main exe.

To implement this:

robbietuk commented 3 weeks ago

Upon further review of apply_normfactors3D, the utility does in fact use the BinNormalisationPETFromComponents class. However, there is currently no setter for the crystal_efficiencies, geometric_factors and block_factorsas the utility uses a streaming method to get the data https://github.com/UCL/STIR/blob/c51067d72efe914d8a344fac913ffc9db0fdba39/src/utilities/apply_normfactors3D.cxx#L91-L97

KrisThielemans commented 3 weeks ago

Yes, the intention was to let all this work via BinNormalisationPETFromComponents. The current file IO is rather ugly and inconvenient, but we'll need to have it for backwards compatibility for a while. In principle, ML_estimate... could directly return a BinNormalisationPETFromComponents object. That needs some careful thought however.

robbietuk commented 3 weeks ago

Yes, the intention was to let all this work via BinNormalisationPETFromComponents.

Do you mean for BinNormalisationPETFromComponents to have functionality to perform ML_estimate... and compute the efficiencies etc?

In principle, ML_estimate... could directly return a BinNormalisationPETFromComponents object. That needs some careful thought however.

It could be as simple as a method on the ML_estimate... class having a method that constructs a BinNormalisationPETFromComponents. I think I am confused as to why architecture is such that the find_ML_normfactors3D and apply_normfactors3D utilities are split.

KrisThielemans commented 2 weeks ago

Yes, the intention was to let all this work via BinNormalisationPETFromComponents.

Do you mean for BinNormalisationPETFromComponents to have functionality to perform ML_estimate... and compute the efficiencies etc?

no, I don't think that's a good idea.

In principle, ML_estimate... could directly return a BinNormalisationPETFromComponents object. That needs some careful thought however.

It could be as simple as a method on the ML_estimate... class having a method that constructs a BinNormalisationPETFromComponents.

yes

I think I am confused as to why architecture is such that the find_ML_normfactors3D and apply_normfactors3D utilities are split.

well, Reconstruction is a different hierarchy than DiscretisedDensity :-). Why would we put "estimation" and "usage" in one class?