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

[SCHEMA] Define common derivatives rules #1072

Merged
merged 25 commits into from
Apr 21, 2022

Conversation

rwblair
Copy link
Member

@rwblair rwblair commented Apr 14, 2022

cherry-picked commits from schema/derivatives branch

@rwblair rwblair requested a review from tsalo as a code owner April 14, 2022 16:23
@codecov
Copy link

codecov bot commented Apr 14, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.50%. Comparing base (dca1564) to head (833074a).

Additional details and impacted files
@@              Coverage Diff               @@
##           schema-sprint    #1072   +/-   ##
==============================================
  Coverage          71.50%   71.50%           
==============================================
  Files                  9        9           
  Lines                930      930           
==============================================
  Hits                 665      665           
  Misses               265      265           

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

@effigies effigies changed the title Schema/common derivatives [SCHEMA] Define common derivatives rules Apr 14, 2022
@effigies effigies added schema Issues related to the YAML schema representation of the specification. Patch version release. derivatives labels Apr 14, 2022
@effigies
Copy link
Collaborator

anat_parametric:
 $ref: schema.rules.datatypes.anat.parametric
 entities:
   $ref: schema.rules.datatypes.anat.parametric.entities
   space: OPTIONAL
   desc: OPTIONAL

…es. Add anatomical volumetric, mask and segmentation derivatives
@@ -0,0 +1,413 @@
---
anat_nonparametric_common:
$ref: schema.rules.datatypes.anat.nonparametric
Copy link
Member

Choose a reason for hiding this comment

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

Part of the goal of #1012 was to eliminate the dependency on the folder structure and filenames for datatype rules. If the group names are unique, then I think we could have schema.rules.datatypes.nonparametric.

@effigies were the names unique?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, the names weren't unique, but we could make them unique if we reorganize. I think fully-qualified is fine as long as we update the paths when things get moved.

desc: OPTIONAL

anat_parametric_common:
$ref: schema.rules.datatypes.anat.parametric
Copy link
Member

Choose a reason for hiding this comment

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

Since the group has an entities field, will it conflict with the entities defined below when the ref is filled out?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The idea is that the below entities will override the original. To make it a patch, we pull in the original entities and patch that. So we go from:

anat_parametric_common:
  $ref: schema.rules.datatypes.anat.parametric
  entities:
    $ref: schema.rules.datatypes.anat.parametric.entities
    space: OPTIONAL
    desc: OPTIONAL

to

anat_parametric_common:
  $ref: schema.rules.datatypes.anat.parametric
  entities:
    subject: required
    session: optional
    run: optional
    acquisition: optional
    ceagent: optional
    reconstruction: optional
    space: optional
    desc: optional

to

anat_parametric_common:
  suffixes:
  - T1map
  - T2map
  - T2starmap
  - R1map
  - R2map
  - R2starmap
  - PDmap
  - MTRmap
  - MTsat
  - UNIT1
  - T1rho
  - MWFmap
  - MTVmap
  - PDT2map
  - Chimap
  - S0map
  - M0map
  extensions:
  - .nii.gz
  - .nii
  - .json
  datatypes:
  - anat
  entities:
    subject: required
    session: optional
    run: optional
    acquisition: optional
    ceagent: optional
    reconstruction: optional
    space: optional
    desc: optional

Comment on lines +24 to +29
SpatialReferenceNoEntity:
selectors:
- dataset.DatasetType == "derivative"
- not(entities contains "space")
fields:
SpatialReference: REQUIRED
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be an inconsistency in the spec. I believe that a missing "space" entity is assumed to be space-orig.

@@ -0,0 +1,413 @@
---
anat_nonparametric_common:
$ref: schema.rules.datatypes.anat.nonparametric
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quick FYI as I browsing the several schema related PRs

starting those keys with a $ will for sure not play nice with MATLAB in the end, unless I change them to something else when converting from yaml to json in the bids-matlab repo but I would prefer to avoid having different flavor of the schema.

Copy link
Collaborator

Choose a reason for hiding this comment

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

_ref would presumably work in all languages and be unlikely to collide with valid keys anywhere we might use this construct.

@rwblair rwblair merged commit f8cc27e into bids-standard:schema-sprint Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
derivatives schema Issues related to the YAML schema representation of the specification. Patch version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants