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

[FIX] clarify participants tsv+json with examples and recommendations #459

Merged
merged 2 commits into from
May 12, 2020

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented Apr 29, 2020

first step towards #458

This PR is intended as a fast intervention to improve the documentation about our participants.tsv.

It's my intention that all changes made here merely clarify and do not prevent us from taking one of the other actions discussed in #458. Yet, I think it's good to make some immediate small improvements and simultaneously think about and discuss long-term higher impact improvements.

This PR is implementing a mix of the suggestions I made in these comments:

In particular, I:

  • say that age, sex, handedness are often used and we thus recommend (!) some values for them
  • stress that a participants.json is highly RECOMMENDED
  • add examples and link to other existing sections

these changes combined with reverting the validator change in https://github.com/bids-standard/bids-validator/pull/905 that @rwblair said he will do in #458 (comment) will hopefully lead to more sane BIDS participants.tsv and json files. --> this will lead to validator WARNINGS when participant.json files are missing.

I agree that there remains much to do, let's keep discussing the general issue in #458 and use this PR to discuss how to make a pragmatic fast-but-small improvement.

Rendered draft

https://2338-150465237-gh.circle-artifacts.com/0/site/03-modality-agnostic-files.html#participants-file

@sappelhoff sappelhoff changed the title FIX: clarify participants tsv [FIX] clarify participants tsv Apr 29, 2020
@nicholst
Copy link
Collaborator

New thought: Age in perinatal populations.

Having just examined a dissertation on imaging of neonates, I learned that there are no less than 4 notions of age used: Gestational age, postmenstrual age, chronological age & corrected age. These are defined in a Policy Statement from the American Academy of Pediatrics (Blackmon et al. (2004)).

According to Table 1 of the policy, typical time units vary for each (weeks is used by all measures, months or years is used mainly with chronological and postmenstrual age).

I was about to suggest an edit but I'm not sure where to put it.

Do we say

For perinatal subjects, please do not use age, but instead define a variable, one of gestational_age, postmenstrual_age, chronological_age or corrected_age, but with time units of years.

Or do we just add to the current age guidelines:

age: numeric value in years (float or integer value). For perinatal subjects, age must still be in years, and the type of age stored in the JSON file, one of gestational age, postmenstrual age, chronological age or corrected age, as per Blackmon et al. (2004).

Reference

Blackmon, L. R., Batton, D. G., Bell, E. F., Denson, S. E., Engle, W. A., Kanto, W. P., … Stark, A. (2004). Age terminology during the perinatal period. Pediatrics, 114(5), 1362–1364. https://doi.org/10.1542/peds.2004-1915

@satra
Copy link
Collaborator

satra commented Apr 29, 2020

@nicholst - this came up in the main discussion. i have added some wording to this change at least to enumerate that issue.

compulsory column `participant_id` that consists of `sub-<label>`,
followed by a list of optional columns describing participants. Each participant
needs to be described by one and only one row.
- for "female", use one of these values: `female`, `f`, `F`, `FEMALE`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to use the keyword MUST here?

Copy link
Member Author

Choose a reason for hiding this comment

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

see some lines above We RECOMMEND to use the following values

optional columns are defined beyond `age`, `sex`, and `handedness`, such as
`group` in this example.

`participants.json` example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the rationale for having this sidecar file described elsewhere in the document? In the example below, it's not clear to me how one would write the "Levels". Also, why is this even needed when you already have a restricted set of values or age and sex?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes I link to a section where this is described in the sentence above [section on tabular files](02-common-principles.md#tabular-files))

Also, why is this even needed when you already have a restricted set of values or age and sex

with this PR, we only RECOMMEND, not REQUIRE --> as I say in the PR intro, the changes here are a first step that should be ok with both sides of the discussion (those that want to have it strict, and those who want to have it lenient or relating to other standards/ontologies)

we need to discuss in the related issue on how to improve things further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay fair enough. Let's continue the discussion in another issue/PR!

@jasmainak
Copy link
Collaborator

Thanks a lot @sappelhoff for taking a stab at this

nicholst
nicholst previously approved these changes Apr 29, 2020
Copy link
Collaborator

@nicholst nicholst left a comment

Choose a reason for hiding this comment

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

Looks good, especially with @satra's edit to clarify age as chronological age.

Co-Authored-By: Richard Höchenberger <[email protected]>
Co-Authored-By: Satrajit Ghosh <[email protected]>
@dorahermes
Copy link
Member

Will there be a standard option in handedness for ambidexter?

@sappelhoff
Copy link
Member Author

Will there be a standard option in handedness for ambidexter?

If this PR were to be merged, we would RECOMMEND one of a, A, ambidextrous, Ambidextrous, or AMBIDEXTROUS

@dorahermes
Copy link
Member

Thanks for clarifying!

@sappelhoff sappelhoff changed the title [FIX] clarify participants tsv [FIX] clarify participants tsv+json with examples and recommendations May 4, 2020
@sappelhoff
Copy link
Member Author

@nicholst @satra @dorahermes @hoechenberger if you approve of this "first step PR" in its current state, we can merge it.

overall discussion will continue in #458

Copy link
Collaborator

@nicholst nicholst left a comment

Choose a reason for hiding this comment

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

Looks good!

@sappelhoff sappelhoff merged commit 41e2214 into bids-standard:master May 12, 2020
@sappelhoff sappelhoff deleted the agesex branch May 12, 2020 17:34
yarikoptic added a commit to yarikoptic/bids-specification that referenced this pull request May 19, 2020
* origin/master: (404 commits)
  [DOC] Auto-generate changelog entry for PR bids-standard#460
  Apply suggestions from code review
  label -> index
  drop _part-, introduce _split-
  [DOC] Auto-generate changelog entry for PR bids-standard#459
  [DOC] Auto-generate changelog entry for PR bids-standard#465
  fix table
  Update src/99-appendices/06-meg-file-formats.md
  [DOC] Auto-generate changelog entry for PR bids-standard#441
  inject _part into MEG spec
  update entity table
  FIX: clarify _part
  Apply suggestions from code review
  FIX: clarify participants tsv
  [DOC] Auto-generate changelog entry for PR bids-standard#457
  Update Release_Protocol.md
  add pdf steps for release protocol
  FIX: remove BESA from list of restricted keywords
  Remove trailing space
  Add reference to PDF on front page of specification
  ...

Conflicts:
	src/02-common-principles.md - had to meld with my previous wording etc.
satra added a commit to satra/bids-specification that referenced this pull request May 23, 2020
* upstream/master: (113 commits)
  [DOC] Auto-generate changelog entry for PR bids-standard#152
  [DOC] Auto-generate changelog entry for PR bids-standard#467
  Specify that suffix must be alphanumeric
  ENH: make NOT RECOMMENDED stronger (SHOULD NOT) for zero padding for uniqueness
  ENH: Include leading . within definition of the file extension
  ENH: provide an example for a suffix based on an _eeg.vhdr filename
  [DOC] Auto-generate changelog entry for PR bids-standard#477
  [DOC] Auto-generate changelog entry for PR bids-standard#460
  Also ignore users urls on github
  Quote regexp in command line
  [INFRA] linkchecker - ignore github pull and tree URLs
  Apply suggestions from code review
  replace purview with scope
  label -> index
  Apply suggestions from code review
  drop _part-, introduce _split-
  Apply SA feedback and amended to purview
  [DOC] Auto-generate changelog entry for PR bids-standard#459
  Add Domain Expert to Maintainers Group
  [DOC] Auto-generate changelog entry for PR bids-standard#465
  ...
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.

6 participants