Skip to content
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 task metadata to PET #1196

Merged
merged 7 commits into from
Aug 25, 2022
Merged

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented Aug 12, 2022

-->
{{ MACROS___make_metadata_table(
{
"TaskName": ("RECOMMENDED", "If used to denote resting scans, a RECOMMENDED convention is to use labels beginning with `rest`.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually this is REQUIRED for almost all datatypes (except for behavioral), for backward compatibility I suspect it is better to keep it RECOMMENDED.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an extra comment that tries to make it clear that eventhough task is not required for PET for resting scans, if one decided to use it, it would be better to stick to the convention other datatypes use.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it is best to keep recommended, and great that you added the extra comment.

@@ -276,6 +276,27 @@ and a guide for using macros can be found at

All reconstruction-specific parameters that are not specified, but one wants to include, should go into the `ReconMethodParameterValues` field.

#### Task

If the OPTIONAL [`task-<label>`](../99-appendices/09-entities.md#task) is used,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does not having task imply? Just that the scan is resting-state, or something more specific? Like it's meant as a static, rather than functional, scan or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that it's PET it could just be that you are measuring binding potential of some ligand I suspect. Some not exactly static because you have a temporal component but not functional for sure. @mnoergaard ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having a task means rest (to lie still in the scanner and note move), and this is how most of PET scans are carried out. Both for static and dynamic scans.

Copy link
Collaborator

@mnoergaard mnoergaard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - but this will also require a few updates to the validator, right?

@Remi-Gau
Copy link
Collaborator Author

Better but it seems that dumping extra metadata in the json did not trigger any failure in the bids examples repo, so let's open an issue to remember to do it but hell should not break loose if we don't do it right away.

bids-standard/bids-examples#330

@sappelhoff
Copy link
Member

Better but it seems that dumping extra metadata in the json did not trigger any failure in the bids examples repo, so let's open an issue to remember to do it but hell should not break loose if we don't do it right away.

That is because AdditionalProperties is not declared in the JSON schema ... and if not declared, it defaults to "true" -- so you may add any metadata right now.

Still, the metadata should be added to the json schema in the validator

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecov
Copy link

codecov bot commented Aug 25, 2022

Codecov Report

Merging #1196 (fd39d09) into master (db4b0f5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1196   +/-   ##
=======================================
  Coverage   88.23%   88.23%           
=======================================
  Files           6        6           
  Lines        1020     1020           
=======================================
  Hits          900      900           
  Misses        120      120           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@sappelhoff sappelhoff merged commit 8106ee8 into bids-standard:master Aug 25, 2022
@Remi-Gau Remi-Gau deleted the task_pet branch December 6, 2022 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Task metadata for PET
4 participants