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] Make explicit that "task" metadata applies to "beh" modality #804

Merged
merged 14 commits into from
Aug 2, 2021

Conversation

Remi-Gau
Copy link
Collaborator

@Remi-Gau Remi-Gau commented May 22, 2021

Fixes #795

Adds task metadata that exist in other modalities to:

  • behavioral
  • PET

All metadata is RECOMMENDED.
Making TaskName REQUIRED (like it is for other modalities) could break backward compatibility, no?

I used a pretty global approach for beh assuming that these metadata could go in any of JSON flles on can find in this folder: should we have a more fine grain approach to this?


@Remi-Gau
Copy link
Collaborator Author

@melanieganz and @mnoergaard I am adding you as reviewers as this also relates to PET. 😄

@Remi-Gau Remi-Gau changed the title [ENH] task standardization [ENH] task metadata homopgeneization May 22, 2021
@Remi-Gau Remi-Gau changed the title [ENH] task metadata homopgeneization [ENH] task metadata homogeneization May 22, 2021
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.

LGTM, probably something to remind ourselves of in #774

@Remi-Gau
Copy link
Collaborator Author

LGTM, probably something to remind ourselves of in #774

Good point.

Once this is merged I will update #774 (and also the validator ?)

@sappelhoff
Copy link
Member

Mmh yes ... about updating the validator: Would you add it to the events.json schema, or the beh.json schema? The latter doesn't exist yet, but I think it'd be a better choice, and also something to clarify in this PR. WDYT?

Otherwise we'd have to specify it in the events.json schema: https://github.com/bids-standard/bids-validator/blob/master/bids-validator/validators/json/schemas/events.json

@Remi-Gau
Copy link
Collaborator Author

I would be more in favor of creating a beh.json schema.

This will need to happen at some point anyway.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

LGTM. Suggested switching to Stroop instead of rest, since rest produces no behavioral data.

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.

This PR will have to be updated with respect to #774 --> metadata tables are now rendered via macros directly from the schema.

Other than that technical change request, I still approve of this PR

@sappelhoff sappelhoff changed the title [ENH] task metadata homogeneization [ENH] Make explicit that "task" metadata applies to "beh" modality Jul 20, 2021
Co-authored-by: Chris Markiewicz <effigies@gmail.com>
@Remi-Gau
Copy link
Collaborator Author

This PR will have to be updated with respect to #774 --> metadata tables are now rendered via macros directly from the schema.

Updated the PR to use the macros and the schema definitions.

The problem is that now the definitions for TaskName and Instructions both make reference to resting state which does not apply to behavior (see @effigies comments above).

Suggestions: shorten the base definitions and add the "resting state" mentions where necessary (func, eeg, meg, ieeg).

@tsalo does that make sense to you?

@tsalo
Copy link
Member

tsalo commented Jul 28, 2021

👍 That works for me!

@Remi-Gau
Copy link
Collaborator Author

Tempted to add some info in the contributing.md on how to formulate the call to the macros to use the metadata description contained in the schema.
Just so that people are aware that those base description can be extended.

@sappelhoff sappelhoff added the schema Issues related to the YAML schema representation of the specification. Patch version release. label Jul 29, 2021
@sappelhoff
Copy link
Member

Tempted to add some info in the contributing.md on how to formulate the call to the macros to use the metadata description contained in the schema.
Just so that people are aware that those base description can be extended.

I think that's a good idea 👍 we'll have to refer people to such docs quite some times in the future

@sappelhoff
Copy link
Member

Thanks @Remi-Gau 🎉 let's get the validator PR fixed and merged next

@Remi-Gau
Copy link
Collaborator Author

Remi-Gau commented Aug 2, 2021

yup.
in the meantime I will update #839

@Remi-Gau Remi-Gau deleted the remi-task_standardization branch November 15, 2021 05:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"standardization" of task metadata across modality
5 participants