-
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
[SCHEMA] bep012 functional derivatives #1209
base: master
Are you sure you want to change the base?
Conversation
…pecification into schema/bep012
…pecification into schema/bep012
…into schema/bep012
…into schema/bep012
Can't figure out the yamllint error. That line looks a lot like other checks with includes statements |
src/schema/rules/sidecars/derivatives/functional_derivatives.yaml
Outdated
Show resolved
Hide resolved
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 new suffixes need to be defined, and I noticed a few typos, but other than that it looks pretty comprehensive.
The three tough things about this BEP are:
- Columns that use regular expressions. Do we need a new object field for these (e.g.,
pattern
), or is name fine? - One-to-many mapping for columns that use regular expressions. Do we need a new object field for these, or can we assume that any regex-based column can appear multiple times in the same TSV file?
- The new column suffixes that can be applied additively. Do we need a new object type for these? They're not really columns, and they can be applied to a wide range of existing columns.
EDIT: Can you also update your branch to pull in the recent test changes?
- ".nii" | ||
- ".nii.gz" |
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.
- ".nii" | |
- ".nii.gz" | |
- $ref: rules.datatypes.func.func.extensions |
Would this work?
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 it will work, it doesn't seem to cost much to be explicit. Either way, a thing to note is that it's $ref
, not _ref
.
src/schema/rules/datatypes/derivatives/functional_derivatives.yaml
Outdated
Show resolved
Hide resolved
src/schema/rules/datatypes/derivatives/functional_derivatives.yaml
Outdated
Show resolved
Hide resolved
src/schema/rules/datatypes/derivatives/functional_derivatives.yaml
Outdated
Show resolved
Hide resolved
src/schema/rules/datatypes/derivatives/functional_derivatives.yaml
Outdated
Show resolved
Hide resolved
src/schema/rules/datatypes/derivatives/functional_derivatives.yaml
Outdated
Show resolved
Hide resolved
- dataset.DatasetType == "derivative" | ||
- '!(["alff", "falff"].includes(suffix))' | ||
fields: | ||
BandpassFilter: optional |
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.
Per our discussion today, I think this may need a level_addendum
. I assume we'll want a more conventional metadata table instead of the current one, which has a "Required for suffix" column instead of the typical "Requirement Level" column.
a: anatomically derived ROIs (white matter and CSF), | ||
t: temporal variance, w: white matter voxels only, c: CSF voxels only | ||
cosine_x: | ||
name: ^cosine_[0-9a-zA-Z]+$ |
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.
These new regex-based names are new. I think having regex-based names fits, but one other novel aspect is that the columns will have a one-to-many relationship. All other columns and metadata fields have had one-to-one mappings so far (one object definition for each column/field). Do we need an additional field in the object schema to account for this?
Co-authored-by: Taylor Salo <[email protected]>
- "lfcdw" | ||
- "vmhc" | ||
|
||
functional_derivatives_timeseries: |
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.
Missing datatype: func
here and elsewhere.
BandpassFilterOpt: | ||
selectors: | ||
- dataset.DatasetType == "derivative" | ||
- '!(["alff", "falff"].includes(suffix))' | ||
fields: | ||
BandpassFilter: optional | ||
BandpassFilterReq: | ||
selectors: | ||
- dataset.DatasetType == "derivative" | ||
- '["alff", "falff"].includes(suffix)' | ||
fields: | ||
BandpassFilter: required |
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 think we can just do this a a single table:
BandpassFilterOpt: | |
selectors: | |
- dataset.DatasetType == "derivative" | |
- '!(["alff", "falff"].includes(suffix))' | |
fields: | |
BandpassFilter: optional | |
BandpassFilterReq: | |
selectors: | |
- dataset.DatasetType == "derivative" | |
- '["alff", "falff"].includes(suffix)' | |
fields: | |
BandpassFilter: required | |
BandpassFilter: | |
selectors: | |
- dataset.DatasetType == "derivative" | |
- '["alff", "falff"].includes(suffix)' | |
fields: | |
BandpassFilter: | |
level: required | |
level_addendum: for suffixes `alff` and `falff` |
TimeseriesMetadata: | ||
selectors: | ||
- dataset.DatasetType == "derivative" | ||
- suffix == "timeseries" |
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.
This should match any of these suffixes.
@@ -0,0 +1,44 @@ | |||
--- |
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.
timeseries.tsv
can contain a superset of what's in these other files.
--- | |
--- | |
GeneralTimeSeries: | |
selectors: | |
- dataset.DatasetType == "derivative" | |
- suffix == 'timeseries' | |
columns: | |
- trans_x: optional | |
- trans_y: optional | |
- trans_z: optional | |
- rot_x: optional | |
- rot_y: optional | |
- rot_z: optional | |
- framewise_displacement: optional | |
- rmsd: optional | |
- rms: optional | |
- non_steady_state_x: optional | |
- dvars: optional | |
- std_dvars: optional | |
- cosine_x: optional | |
- legendre: optional | |
- compcor: optional | |
- melodic_x: optional | |
additional_columns: allowed |
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
…yaml Co-authored-by: Taylor Salo <[email protected]>
[SCHEMA] Add object definitions for BEP012 Functional Derivatives
@@ -52,6 +52,17 @@ color: | |||
Hexadecimal. Label color for visualization. | |||
type: string | |||
unit: hexadecimal | |||
compcor: | |||
name: '[a|t|w|c]_compcor_cor_[0-9a-zA-Z]+' |
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.
Just FTR (attn @bendichter) something similarish came up in BEP032 context where for different shape
of electrodes we would like to have columns describring the shapes, e.g.
name shape shape_radius shape_width
1 square n/a 1
2 circle 1 n/a
My attempt at schematizing bep012. New in this PR, regex based column names.