-
Notifications
You must be signed in to change notification settings - Fork 161
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
[SCHEMA] Add full object definitions for valid values in schema #919
Conversation
@@ -1705,6 +1733,7 @@ PCASLType: | |||
Type the gradient pulses used in the `"control"` condition: | |||
`"balanced"` or `"unbalanced"`. | |||
type: string | |||
# TODO: Add definitions for these values. |
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.
What do the different PCASLType
values mean?
@@ -2082,6 +2145,7 @@ SampleOrigin: | |||
Values MUST be one of `"blood"`, `"saliva"`, `"brain"`, `"csf"`, | |||
`"breast milk"`, `"bile"`, `"amniotic fluid"`, `"other biospecimen"`. | |||
type: string | |||
# TODO: Add definitions for these values. |
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.
Should we describe the different SampleOrigin
values?
@@ -2387,6 +2453,7 @@ SpoilingType: | |||
Specifies which spoiling method(s) are used by a spoiled sequence. | |||
Accepted values: `"RF"`, `"GRADIENT"` or `"COMBINED"`. | |||
type: string | |||
# TODO: Add definitions for these values. |
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.
What do the different SpoilingType
values mean?
@@ -2468,6 +2535,7 @@ TissueOrigin: | |||
Values MUST be one of `"gray matter"`, `"white matter"`, `"csf"`, | |||
`"meninges"`, `"macrovascular"` or `microvascular`. | |||
type: string | |||
# TODO: Add definitions for these values. |
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.
Should we describe the different TissueOrigin
values?
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #919 +/- ##
==========================================
- Coverage 88.00% 87.83% -0.18%
==========================================
Files 16 16
Lines 1326 1356 +30
==========================================
+ Hits 1167 1191 +24
- Misses 159 165 +6
☔ View full report in Codecov by Sentry. |
I'm not 100% sure about this one. I grabbed values from the coordinate systems appendix.
src/schema/objects/metadata.yaml
Outdated
_NonstandardTemplateCoordSys: | ||
type: string | ||
enum: | ||
- individual: | ||
name: individual | ||
description: | | ||
Participant specific anatomical space (for example derived from T1w and/or T2w images). | ||
This coordinate system requires specifying an additional, participant-specific file to be fully defined. | ||
In context of surfaces this space has been referred to as `fsnative`. | ||
|
||
In order for this space to be interpretable, `SpatialReference` metadata MUST be provided. | ||
- study: | ||
name: study | ||
description: | | ||
Custom space defined using a group/study-specific template. | ||
This coordinate system requires specifying an additional file to be fully defined. | ||
|
||
In order for this space to be interpretable, `SpatialReference` metadata MUST be provided. | ||
_NonTemplateCoordSys: | ||
type: string | ||
enum: | ||
- scanner: | ||
name: scanner | ||
description: | | ||
The intrinsic coordinate system of the original image (the first entry of `RawSources`) | ||
after reconstruction and conversion to NIfTI or equivalent for the case of surfaces and dual volume/surface files. | ||
|
||
The `scanner` coordinate system is implicit and assumed by default if the derivative filename does not | ||
define **any** `space-<label>`. | ||
Please note that `space-scanner` SHOULD NOT be used, | ||
it is mentioned in this specification to make its existence explicit. |
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'm not sure if these values should be enumerated here. I took them from https://bids-specification.readthedocs.io/en/stable/99-appendices/08-coordinate-systems.html#nonstandard-coordinate-system-identifiers and https://bids-specification.readthedocs.io/en/stable/99-appendices/08-coordinate-systems.html#non-template-coordinate-system-identifiers.
Noting that this would be a backwards incompatible change (cf #1013), but easily handled by detecting a list of single-keyed objects to a list of keys. |
for more information, see https://pre-commit.ci
…ification into enum-definitions
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.
The Phase/SliceEncodingDirection values are slightly weirdly worded. Would something like this work?
Alternately, we at least should use phase/slice
in both lines.
Co-authored-by: Chris Markiewicz <[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.
I have reviewed this PR and I approve of it, thanks a lot for this work.
Just one question:
For some items you have added the comment
# TODO: Add definitions for these values.
While other items don't have such a comment, yet they are not converted to the "new form" either, e.g.:
enum:
- IODINE
- GADOLINIUM
- CARBON DIOXIDE
- BARIUM
- XENON
Shouldn't we just add information for all enums (except those that only have "n/a")?
Or is that kind of superfluous? (e.g., for m, cm, mm enums ... meter, centimeter ...)
Thanks for reviewing @sappelhoff!
Those could be enums that were added after I started this PR, or they might even be ones I just missed in my original pass-through.
I think some are self-explanatory (e.g., I don't know if I want to write descriptions for all of the allowed values for the |
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.
ok, great. I am going to merge this today and then do follow up PRs:
- rendering the channel type tables in the modality sections from the schema
- creating a new appendix page rendering ALL channel types (and putting links to this appendix below the modality ch type tables)
- adding remaining enums (or adding comments saying that we won't add them as of now ... for cases where such comments are not currently there already)
Thanks! |
thanks a lot again @tsalo! |
This PR adds new information to the schema, but should not affect how it is rendered in the specification.
will simplify finding a solution for:
channels.tsv
files. #1436I'm not going to write a rendering function based on tags in this PR. I think it can go in a future one.
Changes proposed:
To do:
Think about tackling Render valid value restrictions in tables based on object definitions in schema #912 in this PR as well.Dropped in favor of [ENH] Render valid value restrictions in tables based on object definitions in schema #921.