BinNormalisationWithCalibration and set_up(projdataInfo, ExamInfo): #672

danieldeidda · 2020-08-26T13:45:50Z

added class for calibration BinNormalisation and calibration factor #446 , decay and branching ration corr need to be derived from there;
BinNormalisation::set_up() has a new input, exam_info;
all instances where set_up() is used have been updated;
BinNormalisation::apply() does not need start_time and end_time as inputs, can be extracted from exam_info BinNormalisation::apply should use timing info from ExamInfo #340 . Nedd to modify undo().

1) added class for calibration, decay and branching ration corr need to be derived from there; 2) BinNormalisation::set_up() has a new input, exam_info; 3) all instances where set_up() is used have been updated; 4) BinNormalisation::apply() does not need start_time and end_time as inputs, can be extracted from exam_info. Nedd to modify undo().

KrisThielemans

thanks!

I have a few smallish comments, but then also a worry (which actually affects some of my "inline" comments). First the comments:

best to use set_up(shared_ptr<const ExamInfo>& exam_info_sptr, const shared_ptr<const ProjDataInfo>&). Order is then the same as in other places, and it's best to used a ptr as at some point ExamInfo might become a hierarchy
there's also some in experimental (build with it "on", although not all might be included by default)
I thought we said we'd store the actual calibration_factor and branching_ratio in ExamInfo (certainly the branching_ratio should in the future be set by reading the actual sinogram, not the norm file).
I see no reason to have the BinNormalisationWithCalibration::apply(RelatedViewgrams&) members. BinNormalisation will implement it in terms of get_bin_efficiency (if there is a reason, we obviously still need to do undo as you wrote)
we could provide a backwards compatible apply(ProjData, start, end) just calling the new one, but making it deprecated.
It is a but strange to remove the start/end from apply(ProjData) but not the one with RelatedViewgrams nor get_bin_efficiency. However, I strongly recommend to postpone that to another PR, as really we're mixing 2 things here (i.e. how to set calibration factor, and removing unnecessary start/stop parameters).
will need set/get_calibration_factor and set/get_branching_ratio members (although the latter would be superseded by a set_isotope presumably)

Notes:

some of my "inline" comments assumed that BinNormalisation was derived from ParsingObject already, and therefore I suggested calls to base_type functions. However but it appears it isn't, so I think you can ignore those comments for now (or put in the extra mile and derive BinNormalisation from ParsingObject after all, as is actually recommended).

Now the worry:

How do we see that this fits in the hierarchy actually? The current set-up seems to imply that if you want to do norm with calibration, you need to set up a "chained" bin normalisation, taking the "calibration" and the "normal" one. This would work, but it would be

inconvenient for the user as having to set-it up quite verbosely, but more importantly doesn't immediately fit with letting BinNormalisationFromECAT8 or ...GERDF to read the calibration factor from their input as opposed to the par file.
somewhat inefficient (we'd first get a viewgram, normalise it with normal factors, then multiply with a global factor), as opposed to doing it all in one go.

The alternative approach is to have this a class that actually doesn't have apply/undo members, but a single get_global_norm_factor() (or some similar name) that would multiply calibration_factor, branch_ratio (and later-on decay). BinNormalisationFromECAT8 etc would then be derived from this one, but have to call get_global_norm_factor() themselves in their get_bin_efficiency (see an alternative below). It would mean modifying those classes though. This might be ok though, as they need to read the calibration factor anyway.

Side note: for the non-stationary scanner (SPECT) case, the decay factor is going to be bin-dependent, so I guess we cannot have a global factor after all...

I think the 2nd approach is better as ultimately easier for the user (but a bit more work for you...).

It might be better to isolate all the factor stuff in this class, and force the derived class to take it into account, as opposed to hoping that they will do it. I think we could have therefore something like this

BinNormalisationWithCalibration : BinNormalisation // assuming this is already derived from ParsingObject
{
  protected:
   get_calib_decay_whatever_factor(const Bin&) const; // TODO find a better name
   // needs to be implemented by derived class
   float get_uncalibrated_bin_efficiency(const Bin&) = 0;
   float get_bin_efficiency(const Bin&)
    { return this->get_uncalibrated_bin_efficiency(bin)/get_calib_decay_whatever_factor(bin); }

   void set_calibration_factor(float);
private:
  // provide facility to switch off things?
  // would need to be added to the parsing keywords
  bool use_calibration_factor; // default to true
  bool use_branching_ratio; // default to true
};

BinNormalisationFromECAT8 : 
public RegisteredParsingObject<BinNormalisationFromECAT8, BinNormalisation, BinNormalisationWithCalibration>
{
 // adjust base_type, check that all functions like set_up/set_defaults etc call their base_type version 
 // during parsing, call `set_calibration_factor()`
 // rename get_bin_efficiency to get_uncalibrated_bin_efficiency
  // no other changes required
};

In this approach, BinNormalisationWithCalibration is not a leaf-class, so wouldn't have its own add_start/stop_parsing_key (as I commented).

hmmm.... What do you think?

src/recon_buildblock/BinNormalisation.cxx

src/recon_buildblock/BinNormalisationWithCalibration.cxx

src/include/stir/recon_buildblock/BinNormalisationWithCalibration.h

src/recon_buildblock/BinNormalisationWithCalibration.cxx

src/include/stir/recon_buildblock/BinNormalisation.h

danieldeidda · 2020-09-02T09:34:42Z

Hi, I will take some time to read this carefully, in the meantime I will make step by step the modification you suggested

danieldeidda · 2020-09-02T12:44:08Z

there's also some in experimental (build with it "on", although not all might be included by default)

I run this with experimental on but I don't get an error with the new code

danieldeidda · 2020-09-03T10:19:48Z

some of my "inline" comments assumed that BinNormalisation was derived from ParsingObject already, and therefore I suggested calls to base_type functions. However but it appears it isn't, so I think you can ignore those comments for now (or put in the extra mile and derive BinNormalisation from ParsingObject after all, as is actually recommended).

I think it is derived from ParsingObject trough registeredObjected (which is derived from RegisterObjectBase, which is derived from ParsingObject).

KrisThielemans · 2020-09-03T10:56:33Z

I think it is derived from ParsingObject trough registeredObjected (which is derived from RegisterObjectBase, which is derived from ParsingObject).

ok

1) create operator == and check function for ExamInfo 2) make start_time and end_time conts float 3) apply changes in parsing.

danieldeidda · 2020-09-25T15:18:09Z

will need set/get_calibration_factor and set/get_branching_ratio members (although the latter would be superseded by a set_isotope presumab

In ExamInfo or BinNormalisationWithCalibration

danieldeidda · 2020-10-09T16:01:26Z

@KrisThielemans Did you submit changes or comments here? I remember you were writing but I don't see them.

Anyway I seem to have forgotten to take a note on this but I think I am correct when saying that all get_bin_efficiency() functions will now become get_uncalibrated_bin_efficiency()?

KrisThielemans

forgot to press the "submit" button...

One problem with this PR is that we're going to break SIRF, as it's using the "old" style apply (e.g. here) and set_up (here). Can you keep the old functions in place(only in BinNormalisation) for backwards compatibility? Flag them as \deprecated in their doxygen)

src/include/stir/TimeFrameDefinitions.h

src/include/stir/ExamInfo.h

src/include/stir/recon_buildblock/BinNormalisationWithCalibration.h

src/recon_buildblock/BinNormalisationWithCalibration.cxx

src/include/stir/ExamInfo.inl

danieldeidda · 2020-10-09T16:59:49Z

I remember we said to remove registered_name as we don't use parsing, however I get this error:

/home/nmdicom-recon/CalibNorm/STIR/src/include/stir/RegisteredParsingObject.inl:34: error: ‘registered_name’ is not a member of ‘stir::BinNormalisationWithCalibration’{ return Derived::registered_name; }

I think this happens when I say:

public RegisteredParsingObject<BinNormalisationFromECAT8, BinNormalisation, BinNormalisationWithCalibration>

src/include/stir/recon_buildblock/BinNormalisationWithCalibration.h

danieldeidda · 2020-10-12T13:14:01Z

forgot to press the "submit" button...

One problem with this PR is that we're going to break SIRF, as it's using the "old" style apply (e.g. here) and set_up (here). Can you keep the old functions in place(only in BinNormalisation) for backwards compatibility? Flag them as \deprecated in their doxygen)

Shall we have two functions for set_up() and apply() or shall we make the new one a property of the derived classes?

…ionWithCalibration

…libration; change all get__bin_efficiency() with get_uncalibrated_bin_efficiency().

KrisThielemans · 2020-10-12T16:19:07Z

ah, ok. I didn't think you'd do the ECAT7 etc code now (as ideally we'd then read the calibration factor etc from the header), but fine.

I'm not sure what to do with the ChainedBinNormalisation though. In normal practice, one of these will be attenuation, the other "true" normalisation. It's the latter that would be derived from ...BinNormalisationWithCalibration. I feel therefore that ChainedBinNormalisation should not be derived from BinNormalisationWithCalibration. That sounds simple enough, but the difficulty becomes if we want to store the calibration factor etc in the image after recon. It seems we need to have get_calibration_factor() to be moved up to BinNormalisation for that. But it's not clear to me how that would work... sorry.

KrisThielemans · 2020-10-12T16:21:04Z

The restarted Travis jobs are still timing-out. Did you run the recon_test_pack locally? This is likely the "infinite recursion" in operator== though, so it might be fixed now. let's see...

src/recon_buildblock/BinNormalisationWithCalibration.cxx

src/include/stir/recon_buildblock/BinNormalisationWithCalibration.h

Co-authored-by: Kris Thielemans <[email protected]>

KrisThielemans · 2020-10-12T16:46:04Z

I've cancelled most Travis and Appveyor jobs, to counter global warming...

KrisThielemans · 2020-10-13T09:17:41Z

travis says that recon_test_pack/run_simulation.sh fails.

danieldeidda · 2020-10-13T10:33:45Z

travis says that recon_test_pack/run_simulation.sh fails.

let me have a look

danieldeidda · 2020-10-13T14:14:54Z

travis says that recon_test_pack/run_simulation.sh fails.

This is the problem: ERROR: BinNormalisation set-up with different ExamInfo.

interesting something wrong with the operator ==

KrisThielemans · 2020-10-13T14:19:20Z

I guess should let it print parameter_info if both if this happens. Would make it easier to diagnose.

danieldeidda · 2020-10-13T14:30:58Z

They appear to be the same 👍
Set_up was with
Modality: PT
Patient position: unknown
Scan start time: 0
Time frame start - end (duration), all in secs: 0 - 0 (0)
number of energy windows:=1
energy window lower level[1] := -1
energy window upper level[1] := -1

Called with
Modality: PT
Patient position: unknown
Scan start time: 0
Time frame start - end (duration), all in secs: 0 - 0 (0)
number of energy windows:=1
energy window lower level[1] := -1
energy window upper level[1] := -1

danieldeidda · 2020-12-08T17:11:12Z

I am thinking we need to read the cross calibration factor as well and set our calibration factor to quantification*cross_calibration

I feel that their header implies that we don't need the cross_calibration (which should have to do with well-counter to scanner), but there is essentially no way of knowing this without either asking Siemens, or running e7tools with different headers and seeing what it does.

Given all this uncertainty, I suggest we don't include use the factor yet (i.e. return 1), but just read it, and create an issue for it. Otherwise, people's images are now going to change, and with the next PR, they will change again.

All this is of course the reason I originally suggested to postpone putting the calibration factor in for ECAT8 etc 😉

seems the same for ECAT7:

// now fill in what we can
  mhead.calibration_factor = 1.F;
  mhead.well_counter_factor=1.F;

setting it to 1

…ibration_factor is set in the parameterfile the calibration factor is overwritten

danieldeidda · 2020-12-09T16:29:14Z

is this going to release 4.1?

KrisThielemans · 2020-12-09T19:36:26Z

No. As it brings backwards compatibility on the code level

danieldeidda · 2020-12-10T15:18:29Z

I am thinking that BinNormFromProjData also may need to be derived from BinNormWitCalib or else in case like the Mediso scanner where the normalisation comes from projData the factor will not be applied. I do not see any drawback of doing this but maybe I am wrong.

danieldeidda · 2020-12-10T15:22:03Z

No. As it brings backwards compatibility on the code level

OK. shall we add something in the userguide? I think we said that this should go togheter with the normalisation section.

KrisThielemans · 2020-12-10T21:35:07Z

shall we add something in the userguide? I think we said that this should go togheter with the normalisation section.

We do need to add something, but I'm not 100% sure where. We probably need a new section on image units, which right now is going to be a bit sketchy.

proportional to counts, proportionality factor fixed in terms of sum of along horizontal direction of voxel values (therefore scaling with voxel size))
if normalisation supports calib factor, then will be taken into account (is it mult or divide?). value reported in header for file formats that support it (currently only Interfile)

KrisThielemans · 2020-12-10T21:43:44Z

I am thinking that BinNormFromProjData also may need to be derived from BinNormWitCalib or else in case like the Mediso scanner where the normalisation comes from projData the factor will not be applied. I do not see any drawback of doing this but maybe I am wrong.

I'm a bit reluctant to do that as it is also used by many to store ACFs. some remarks:

how do we know which is which? I guess could default calib factor to -1?
I would not multiply with it in the bin-efficiency calculation (i.e. assume it has been done already). just use it for recording. this is how it would come out in the correct_projdata with an ECAT8 norm anyway.
it seems to imply you then need to store the calib factor in the "norm" projection data (as where are we going to get it from otherwise)? I suppose this is possible as you could have it for all projdata to signify if they have been calibrated. However, this seems to lead us quite far with quite a lot of other work.

I don't think I'd do this in this PR therefore (or at all).

Any reason you cannot create a BinNormalisation class for your Mediso data?

danieldeidda · 2020-12-11T11:58:41Z

I am thinking that BinNormFromProjData also may need to be derived from BinNormWitCalib or else in case like the Mediso scanner where the normalisation comes from projData the factor will not be applied. I do not see any drawback of doing this but maybe I am wrong.

I'm a bit reluctant to do that as it is also used by many to store ACFs. some remarks:
* how do we know which is which? I guess could default calib factor to -1?

* I would not multiply with it in the bin-efficiency calculation (i.e. assume it has been done already). just use it for recording. this is how it would come out in the correct_projdata with an ECAT8 norm anyway.

* it seems to imply you then need to store the calib factor in the "norm" projection data (as where are we going to get it from otherwise)? I suppose this is possible as you could have it for all projdata to signify if they have been calibrated. However, this seems to lead us quite far with quite a lot of other work.
I don't think I'd do this in this PR therefore (or at all).

Any reason you cannot create a BinNormalisation class for your Mediso data?

OK, no I suppose I could create a BinNormalisationFromMediso

danieldeidda · 2020-12-11T11:59:42Z

shall we add something in the userguide? I think we said that this should go togheter with the normalisation section.

We do need to add something, but I'm not 100% sure where. We probably need a new section on image units, which right now is going to be a bit sketchy.
* proportional to counts, proportionality factor fixed in terms of sum of along horizontal direction of voxel values (therefore scaling with voxel size))

* if normalisation supports calib factor, then will be taken into account (is it mult or divide?). value reported in header for file formats that support it (currently only Interfile)

So, shall we do it in another PR?

KrisThielemans · 2020-12-11T12:05:50Z

shall we do it in another PR?

fine for me. Do you have something in the release notes now? Probably important for people to know this has happened. (could even say in the release notes that documentation is pending, and we edit that in the next PR).

danieldeidda · 2020-12-11T12:07:59Z

shall we do it in another PR?

fine for me. Do you have something in the release notes now? Probably important for people to know this has happened. (could even say in the release notes that documentation is pending, and we edit that in the next PR).

I was thinking to add it but not sure in which version, 5.0?

KrisThielemans · 2020-12-11T13:16:46Z

I was thinking to add it but not sure in which version, 5.0?

yes please

danieldeidda · 2020-12-11T14:56:14Z

I think this is ready to go

src/buildblock/TimeFrameDefinitions.cxx

Co-authored-by: Kris Thielemans <[email protected]>

danieldeidda

cool

KrisThielemans requested changes Aug 28, 2020

View reviewed changes

set_up() has a shared pointer for examInfo instead of the object

d12a26d

danieldeidda added 2 commits September 25, 2020 12:13

BinNormalisation:

2e6a1bc

1) create operator == and check function for ExamInfo 2) make start_time and end_time conts float 3) apply changes in parsing.

Merge branch 'master' into CalibNorm

6df4123

KrisThielemans requested changes Oct 9, 2020

View reviewed changes

KrisThielemans reviewed Oct 10, 2020

View reviewed changes

src/include/stir/recon_buildblock/BinNormalisationWithCalibration.h Outdated Show resolved Hide resolved

danieldeidda added 2 commits October 12, 2020 16:49

create required == operators and applie corrections to BinNormalisat…

877f967

…ionWithCalibration

make GE, ECAT7 and ECAT8 BinNorm derived from BinNormalisationWithCa…

6a10fd1

…libration; change all get__bin_efficiency() with get_uncalibrated_bin_efficiency().

KrisThielemans requested changes Oct 12, 2020

View reviewed changes

src/recon_buildblock/BinNormalisationWithCalibration.cxx Outdated Show resolved Hide resolved

src/recon_buildblock/BinNormalisationWithCalibration.cxx Outdated Show resolved Hide resolved

src/include/stir/recon_buildblock/BinNormalisationWithCalibration.h Outdated Show resolved Hide resolved

danieldeidda and others added 3 commits October 12, 2020 17:36

Update src/recon_buildblock/BinNormalisationWithCalibration.cxx

b465e7d

Co-authored-by: Kris Thielemans <[email protected]>

Update src/recon_buildblock/BinNormalisationWithCalibration.cxx

2c6ee17

Co-authored-by: Kris Thielemans <[email protected]>

set_radionuclide() accepts a string not a float

2564bc5

set_radionuclide() accepts a string not a float

66b1b6c

danieldeidda added 2 commits December 8, 2020 16:33

BinNormFromECAT8: read calibration factor but set to 1 for now

1cd561c

BinNormFromGEHDF5: t set to 1 for now

f769449

danieldeidda added 3 commits December 8, 2020 17:12

BinNormFroMECAT7: set to 1 for now

75d2444

BinNormFromSPECT: set to value found in the header or if measured_cal…

4cfb208

…ibration_factor is set in the parameterfile the calibration factor is overwritten

error thrown if radioniclide was defined butis not 18F

9ba0f6d

danieldeidda added 3 commits December 11, 2020 14:37

[ci skip] add new functionalities and backwards incompatibilities

7aa75fc

[ci skip] add new functionalities and backwards incompatibilities

ed2a967

Merge branch 'master' into CalibNorm

ca50cd6

minor updates to release_5.0.htm regarding binnormalisation

1fd1724

KrisThielemans requested changes Dec 11, 2020

View reviewed changes

src/buildblock/TimeFrameDefinitions.cxx Outdated Show resolved Hide resolved

Update src/buildblock/TimeFrameDefinitions.cxx

26c25ed

Co-authored-by: Kris Thielemans <[email protected]>

danieldeidda commented Dec 11, 2020

View reviewed changes

KrisThielemans merged commit 72ba889 into UCL:master Dec 12, 2020

danieldeidda deleted the CalibNorm branch December 12, 2020 17:14

KrisThielemans mentioned this pull request Dec 15, 2020

STIR master changes in BinNormalisation break SIRF SyneRBI/SIRF#838

Closed

KrisThielemans mentioned this pull request Jan 6, 2021

TOF SyneRBI/SIRF#543

Merged

KrisThielemans mentioned this pull request Jan 20, 2021

BinNormalisationFromECAT8 broken #804

Closed

BinNormalisationWithCalibration and set_up(projdataInfo, ExamInfo): #672

BinNormalisationWithCalibration and set_up(projdataInfo, ExamInfo): #672

Uh oh!

Conversation

danieldeidda commented Aug 26, 2020

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danieldeidda commented Sep 2, 2020

Uh oh!

danieldeidda commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danieldeidda commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Sep 3, 2020

Uh oh!

danieldeidda commented Sep 25, 2020

Uh oh!

danieldeidda commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danieldeidda commented Oct 9, 2020

Uh oh!

Uh oh!

danieldeidda commented Oct 12, 2020

Uh oh!

KrisThielemans commented Oct 12, 2020

Uh oh!

KrisThielemans commented Oct 12, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

KrisThielemans commented Oct 12, 2020

Uh oh!

KrisThielemans commented Oct 13, 2020

Uh oh!

danieldeidda commented Oct 13, 2020

Uh oh!

danieldeidda commented Oct 13, 2020

Uh oh!

KrisThielemans commented Oct 13, 2020

Uh oh!

danieldeidda commented Oct 13, 2020

Uh oh!

danieldeidda commented Dec 8, 2020

Uh oh!

danieldeidda commented Dec 9, 2020

Uh oh!

KrisThielemans commented Dec 9, 2020

Uh oh!

danieldeidda commented Dec 10, 2020

Uh oh!

danieldeidda commented Dec 10, 2020

Uh oh!

KrisThielemans commented Dec 10, 2020

danieldeidda commented Sep 2, 2020 •

edited

Loading

danieldeidda commented Sep 3, 2020 •

edited

Loading

danieldeidda commented Oct 9, 2020 •

edited

Loading