-
Notifications
You must be signed in to change notification settings - Fork 161
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
[ENH] add metadata to PET calibration factor: "DoseCalibrationFactor" #825
Conversation
I am all in for more clarity about the data description but what is this "calibration factor" about? When/where would it be useful to know about this? |
|
Hi @CPernet, |
yes, thx @mnoergaard - I knew you'd be the man for a good review :-) note that in ecat (HRRT) this is not applied to the data, but stored separately - I'll do the suggested changes thx (and yes hard to see why you would want count/sec in general, but maybe for some more method work) |
@mnoergaard new commit :-) |
This looks good to me - thanks @CPernet. Next we need to update the validator in a separate PR. |
@ChristophePhillips @mnoergaard you need to 'approve' in the review |
@@ -213,6 +213,7 @@ We refer to the common principles for the standards for describing dates and tim | |||
| ScaleFactor | RECOMMENDED | [array][] of [numbers][] | Scale factor for each frame. | | |||
| ScatterFraction | RECOMMENDED | [array][] of [numbers][] | Scatter fraction for each frame (Units: 0-100%). | | |||
| DecayCorrectionFactor | RECOMMENDED | [array][] of [numbers][] | Decay correction factor for each frame. | | |||
| DoseCalibrationFactor | RECOMMENDED | [number][] | Value used to calibrate radioactivity count, i.e. transform to meaningful unit (e.g. Becquerel) - value already applied to the data as a multiplication factor that scales them from counts/sec to Bq/ml (dicom tag 0054,1322). | |
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 have problems understanding the description. Could you please split this up into sentences with each sentence conveying a meaning?
Let me clarify what I find confusing:
Value used to calibrate radioactivity count
--> is "radioactivity count" metadata that corresponds to some already existing field? If yes, can you reference that one, please?
- value already applied to the data as a multiplication factor that scales them from counts/sec to Bq/ml (dicom tag 0054,1322).
does that connect to the previous sentence? Or is it a standalone statement? Or an example / clarification?
Lastly:
- We have agreed not to use latin abbreviations in the BIDS specification, see: https://the-turing-way.netlify.app/community-handbook/style.html#avoid-latin-abbreviation --> so please remove/replace the latin
- The table pipes of this table need to be aligned (currently not the case, hence CIs are failing)
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.
Thanks for pointing this out @sappelhoff. I have now updated the description, so it is very much in line with the dicom description, and then referring to the dicom tag. Then it is possible to make a dicom lookup, if one wants to search for more information. This also removes the latin abbreviations, and then I have aligned the table pipes.
1. Updated text for DoseCalibrationFactor to be in correspondence with dicom description 2. removed latin phrasing 3. fix table aligment
thx @sappelhoff pushed it (sorry on top of martin, updating at the same time) |
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 made a slight change in wording, see: 7319961
Other than that, it looks fine to me - given that a couple of more people look at it and agree that we need it.
@sappelhoff this is now approved - the only bug I see is a link related to ASL??? |
Yes there has been some fluctuation with the OSIPI URL the last days https://www.osipi.org cc @HenkMutsaerts I haven't merged this yet so that more people may see it and chime in before it goes in --> in accordance with our 5-day rule: https://github.com/bids-standard/bids-specification/blob/master/DECISION-MAKING.md#rules Sometimes maintainers can break that rule according to their own judgment, but in this case, I think there is no harm done leaving it for other people to see for two more days. Unless I hear otherwise, let's merge this on Saturday evening or Sunday. |
Thanks @CPernet for getting this started and everybody else for the reviews! |
As we are working on MATLAB and Python nibabel converter - we realize quantitative PET means multiplying data by the scanner calibration factor ; while not essential, it would be nice to have this information in the json file (in the table as a recommended tag)