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] Allow Levels field of column descriptions to be objects with TermURLs for each level #1603

Merged
merged 7 commits into from
Sep 11, 2023

Conversation

Remi-Gau
Copy link
Collaborator

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
@Remi-Gau
Copy link
Collaborator Author

@surchs @ericearl @alyssadai

@Remi-Gau
Copy link
Collaborator Author

@effigies am I correct that this is one aspect of the spec that has not been schemaified yet?

@codecov
Copy link

codecov bot commented Aug 29, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (fc98780) 87.83% compared to head (c1da9bb) 87.83%.
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1603   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files          16       16           
  Lines        1356     1356           
=======================================
  Hits         1191     1191           
  Misses        165      165           

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

@Remi-Gau
Copy link
Collaborator Author

Should the examples of the json sidecar for participants.tsv and for phenotypic assessments be updated to also include this way of describing levels?

@@ -513,6 +513,28 @@ Example:
}
```

Each level can be described with a string as in the example above,
or with an object containing the fields `Description` and `TermURL`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect BIDS users to know what TermURL expects? How about:

Suggested change
or with an object containing the fields `Description` and `TermURL`
or with an object containing the fields `Description` and `TermURL`.
If you use the object format, the value of the `TermURL` key should
be a URL to a term from a controlled vocabulary that describes the
level. For example: https://www.ncbi.nlm.nih.gov/mesh/68008297 for
"male". The value of the `Description` key should then correspond
to the human readable name of this term (or to the preferred name
if several options exist in the controlled vocabulary). For example: "Male".

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we're going to improve the definition of TermURL, we should do it in https://bids-specification.readthedocs.io/en/stable/glossary.html#termurl-metadata so it appears in the glossary and any tables it's defined in.

https://bids-specification.readthedocs.io/en/stable/glossary.html#termurl-metadata

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could only transfer one sentence to the TermURL generic definition, the rest of @surchs suggestion feels too specific to this example I think.

src/common-principles.md Outdated Show resolved Hide resolved
src/common-principles.md Outdated Show resolved Hide resolved
@effigies
Copy link
Collaborator

@effigies am I correct that this is one aspect of the spec that has not been schemaified yet?

Correct. It's a weird state because what we really need to say is that, if the suffix is TSV, then additional properties of sidecars have the form of an object with certain allowed terms. It would have been much more convenient if we had made the column descriptions an object in a single field. (Further, there generally aren't rules against extra metadata, so it's actually that we need to create a typed metadata field dynamically based on the headers in the TSV.)

Remi-Gau and others added 4 commits August 29, 2023 23:23
Co-authored-by: Sebastian Urchs <surchs@users.noreply.github.com>
@Remi-Gau Remi-Gau marked this pull request as ready for review September 6, 2023 15:38
Copy link
Contributor

@surchs surchs left a comment

Choose a reason for hiding this comment

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

🎉 thanks a lot @Remi-Gau. One free bagel on us!

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! Thanks a lot @Remi-Gau

@Remi-Gau
Copy link
Collaborator Author

thanks @sappelhoff @surchs @effigies for the reviews

@Remi-Gau Remi-Gau merged commit 36e2e3c into bids-standard:master Sep 11, 2023
19 checks passed
@Remi-Gau Remi-Gau deleted the enh/object_for_levels branch September 11, 2023 14:18
@effigies effigies changed the title [ENH] allow Levels values to be objects [ENH] Allow Levels field of column descriptions to be objects with TermURLs for each level Sep 11, 2023
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.

allow Levels values (in sidecar for TSV files) to be objects
4 participants