-
Notifications
You must be signed in to change notification settings - Fork 169
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
[ENH] Allow Levels field of column descriptions to be objects with TermURLs for each level #1603
Conversation
Remi-Gau
commented
Aug 29, 2023
- closes allow Levels values (in sidecar for TSV files) to be objects #1573
@effigies am I correct that this is one aspect of the spec that has not been schemaified yet? |
Codecov ReportPatch and project coverage have no change.
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. |
Should the examples of the json sidecar for participants.tsv and for phenotypic assessments be updated to also include this way of describing levels? |
src/common-principles.md
Outdated
@@ -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` |
There was a problem hiding this comment.
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:
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". |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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.) |
Co-authored-by: Sebastian Urchs <[email protected]>
There was a problem hiding this 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!
There was a problem hiding this 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
thanks @sappelhoff @surchs @effigies for the reviews |