[FIX] PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor. #1021

bendhouseart · 2022-03-03T18:45:33Z

Small updates to the PET spec:

Added references/correspondences between all known dicom tags relating to PET entities
Clarified scale factor to, hopefully, avoid double scaling of images converted to nifti.
Fixed typo on injection start
Updated citation to include PET BIDS Paper

src/04-modality-specific-files/09-positron-emission-tomography.md

Added dicom tag description for `InjectionStart`

CPernet

good to merge IMO assuming i did not forget brackets or commas :-)

tsalo

There are some minor formatting issues with the macro calls, and I have one question about the updated definition for ScaleFactor. Thanks!

src/04-modality-specific-files/09-positron-emission-tomography.md

tsalo · 2022-03-04T15:54:34Z

src/schema/objects/metadata.yaml

@@ -2293,7 +2293,7 @@ SamplingFrequency:
 ScaleFactor:
  name: ScaleFactor
  description: |
-    Scale factor for each frame.
+    Scale factor for each frame. Values must be set if the imaging data (.nii) are unscaled, otherwise set to 1.


Could you explain this a bit for me? I don't do PET, so I'm a little confused. It sounds like you're saying that this field is REQUIRED if the data are unscaled, but also required if they aren't.

I'm going to have to refresh my memory on this as well, but I seem to recall that we ran into some double scaling occurring as a result of converting to nifti (this smells of ECAT). And we wanted to preserve that scaling history in some way in the spec.

I can see how you would need the scaling factor in that case, but I'm still confused about what the update to the description is meant to convey.

it makes explicit something that was implicit - some conversion wrote the scaling values in the json and applied to the data then what should a user do then? with that change we know

I'm still confused. Perhaps the source of my confusion will be clearer if I break the added sentence down.

Part 1: "Values must be set if the imaging data (.nii) are unscaled"

My interpretation: If the data are unscaled, ScaleFactor is required.

Part 2: "otherwise set to 1."

My interpretation: If the data are scaled, ScaleFactor is required.

The combined interpretation would thus be, regardless of whether the data are scaled or not, ScaleFactor is required. But then, in the table, ScaleFactor isn't required, it's recommended.

I think the concern is that by updating ScaleFactor to required will break the few currently passing datasets that are out there. @mnoergaard had some concerns regarding this so I will let him weigh in on this.

You're right, that would break backwards compatibility. How about something like this?

This field MUST be defined if the imaging data (.nii[.gz]) are scaled. If this field is not defined, then it is assumed that the scaling factor is 1. Defining this field when the scaling factor is 1 is recommended, for the sake of clarity.

I'd be in favor of such a suggestion - thanks @tsalo! @CPernet what do you think of this?

yep perfect thx @tsalo

Updated here, should be gtg.

Co-authored-by: Taylor Salo <[email protected]>

- Fixed markdown formatting in src/01-introduction.md - Removed dicom tag description for InjectionTimeStart in src/04-modality-specific-files/09-positron-emission-tomography.md as there is some disagreement on whether dicom tag Radiopharmaceutical Start Time (0018, 1072) should be referenced.

effigies · 2022-03-08T16:13:24Z

src/04-modality-specific-files/09-positron-emission-tomography.md

@@ -154,8 +154,8 @@ which we divide into several categories:

 {{ MACROS___make_metadata_table(
   {
-      "TracerName": "REQUIRED",
-      "TracerRadionuclide": "REQUIRED",
+      "TracerName": ("REQUIRED", "Corresponds to DICOM Tags (0008,0105) `Mapping Resource` and (0008,0122) `Mapping Resource Name`."),


Why aren't we putting these in the metadata.yaml directly?

well some were in yaml some were in spec, and there is no rule to decide --so being lazy i pushed all new ones there

The rule I would use is whether the BIDS term unambiguously corresponds to a DICOM tag (so would be sensible to render any time the term is rendered), or if it corresponds to a DICOM tag in the specific context of a particular table.

I'm not confident enough to say that these tags should belong in the metadata.yml as many of these terms are often more explicitly defined (or known....) by the experiment designer/performer or simply not present in the dicom headers. Adding them to the tables provides a bread crumb for a user to extract this info if they're not omnipotent concerning the experiment or the metadata.

"Corresponds to DICOM Tag" doesn't say to me "MUST be derived from DICOM Tag". I think linking the concepts in the term definition helps clarify.

That said, I won't die on this hill. We can move these into metadata.yaml in the future as well.

"Corresponds to DICOM Tag" doesn't say to me "MUST be derived from DICOM Tag". I think linking the concepts in the term definition helps clarify.

That said, I won't die on this hill. We can move these into metadata.yaml in the future as well.

Agreed. Have already noticed in this PR that it's very easy 🙄 to add something into these tables and duplicate what's in the metadata.yaml or vice versa.

Update Scale Factor description to consensus

src/schema/objects/metadata.yaml

sappelhoff

+1 to merge once my minor suggestions have been incorporated.

sappelhoff · 2022-03-29T16:07:37Z

@tsalo you still have a "changes requested" blocking review. Could you please confirm that your concerns have been addressed (seems like it) and re-review?

tsalo

LGTM!

mnoergaard

LGTM! thanks!

Co-authored-by: Stefan Appelhoff <[email protected]>

sappelhoff · 2022-03-31T18:49:02Z

Thanks @bendhouseart and @CPernet and all reviewers :-)

codecov · 2024-08-28T05:31:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.50%. Comparing base (6043989) to head (6744126).
Report is 1464 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1021       +/-   ##
===========================================
+ Coverage   34.05%   71.50%   +37.45%     
===========================================
  Files           8        9        +1     
  Lines         834      930       +96     
===========================================
+ Hits          284      665      +381     
+ Misses        550      265      -285

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sappelhoff · 2024-08-28T07:53:08Z

#1021 (comment)

very funny to receive a codecov report here, more than two years after the last push 🤔

CPernet added 4 commits March 3, 2022 15:31

PET citation update

37a0c29

DICOM tag error fix

472a9ad

scale factor should be 1 (does it work as 'array')

fde28fa

added all DICOM tags we know

bf719af

bendhouseart commented Mar 3, 2022

View reviewed changes

src/04-modality-specific-files/09-positron-emission-tomography.md Outdated Show resolved Hide resolved

Update 09-positron-emission-tomography.md

b826fa5

Added dicom tag description for `InjectionStart`

CPernet reviewed Mar 4, 2022

View reviewed changes

CPernet marked this pull request as ready for review March 4, 2022 08:43

CPernet requested a review from tsalo as a code owner March 4, 2022 08:43

tsalo requested changes Mar 4, 2022

View reviewed changes

bendhouseart and others added 9 commits March 4, 2022 11:01

Update src/04-modality-specific-files/09-positron-emission-tomography.md

56b9b36

Co-authored-by: Taylor Salo <[email protected]>

Update src/04-modality-specific-files/09-positron-emission-tomography.md

1a4c366

Co-authored-by: Taylor Salo <[email protected]>

Update src/04-modality-specific-files/09-positron-emission-tomography.md

12a11f8

Co-authored-by: Taylor Salo <[email protected]>

Update src/04-modality-specific-files/09-positron-emission-tomography.md

bedf7f5

Co-authored-by: Taylor Salo <[email protected]>

Update src/04-modality-specific-files/09-positron-emission-tomography.md

fd8b8f4

Co-authored-by: Taylor Salo <[email protected]>

Update src/04-modality-specific-files/09-positron-emission-tomography.md

c7ad680

Co-authored-by: Taylor Salo <[email protected]>

Update src/04-modality-specific-files/09-positron-emission-tomography.md

2c93017

Co-authored-by: Taylor Salo <[email protected]>

added back tag to description, dropped reference to timezero

25eb9bf

effigies reviewed Mar 8, 2022

View reviewed changes

CPernet requested a review from tsalo March 11, 2022 14:58

CPernet mentioned this pull request Mar 11, 2022

DICOM tags yaml vs md #1028

Open

Update metadata.yaml

6b94636

Update Scale Factor description to consensus

bendhouseart requested review from mnoergaard and CPernet March 17, 2022 01:21

removed trailing spaces.

b8f50e3

sappelhoff changed the title ~~[FIX] - PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor.~~ [FIX] PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor. Mar 29, 2022

sappelhoff reviewed Mar 29, 2022

View reviewed changes

src/schema/objects/metadata.yaml Outdated Show resolved Hide resolved

sappelhoff approved these changes Mar 29, 2022

View reviewed changes

tsalo approved these changes Mar 29, 2022

View reviewed changes

mnoergaard approved these changes Mar 29, 2022

View reviewed changes

update text "recommended" to "RECOMMENDED"

6744126

Co-authored-by: Stefan Appelhoff <[email protected]>

sappelhoff merged commit 4428fd3 into bids-standard:master Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor. #1021

[FIX] PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor. #1021

bendhouseart commented Mar 3, 2022

CPernet left a comment •

edited

Loading

tsalo left a comment

tsalo Mar 4, 2022

bendhouseart Mar 4, 2022

tsalo Mar 4, 2022

CPernet Mar 5, 2022

tsalo Mar 10, 2022

bendhouseart Mar 15, 2022

tsalo Mar 15, 2022 •

edited

Loading

mnoergaard Mar 15, 2022

CPernet Mar 16, 2022

bendhouseart Mar 17, 2022

effigies Mar 8, 2022

CPernet Mar 8, 2022

effigies Mar 8, 2022

bendhouseart Mar 8, 2022

effigies Mar 8, 2022

bendhouseart Mar 8, 2022

sappelhoff left a comment

sappelhoff commented Mar 29, 2022

tsalo left a comment

mnoergaard left a comment

sappelhoff commented Mar 31, 2022

codecov bot commented Aug 28, 2024

sappelhoff commented Aug 28, 2024

[FIX] PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor. #1021

[FIX] PET Spec; added known DICOM tags, fixed tag error, updated citation, clarified scale factor. #1021

Conversation

bendhouseart commented Mar 3, 2022

CPernet left a comment • edited Loading

Choose a reason for hiding this comment

tsalo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsalo Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sappelhoff left a comment

Choose a reason for hiding this comment

sappelhoff commented Mar 29, 2022

tsalo left a comment

Choose a reason for hiding this comment

mnoergaard left a comment

Choose a reason for hiding this comment

sappelhoff commented Mar 31, 2022

codecov bot commented Aug 28, 2024

Codecov Report

sappelhoff commented Aug 28, 2024

CPernet left a comment •

edited

Loading

tsalo Mar 15, 2022 •

edited

Loading