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] Change modalities to instruments #1237

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Aug 19, 2022

Closes #593 and possibly closes #1141. The problem that this PR attempts to address is that the definition of "modality" in Common Principles conflicts with its use in the schema.

Related proposal from Robert Oostenveld: https://docs.google.com/document/d/1cY-iZL8rtgDJ1-YVs1f2h5ugOiuK144zu4ju9UXZEGY/edit

Changes proposed:

  • Rename schema/objects/modalities.yaml to schema/objects/instruments.yaml.
  • Rename schema/rules/modalities.yaml to schema/rules/instruments.yaml.
  • Add a new "Instrument" element to the Common Principles.
  • Reference instrument in rules, rather than modality.
  • Change any cases where modality is checked against EEG or iEEG to "bioamp"

@tsalo tsalo added opinions wanted Please read and offer your opinion on this matter discussion ongoing discussion schema Issues related to the YAML schema representation of the specification. Patch version release. schema-structure Changes to the fundamental organization/structure of the YAML schema. Minor version release. labels Aug 19, 2022
@codecov
Copy link

codecov bot commented Aug 19, 2022

Codecov Report

Merging #1237 (19762bc) into master (69e1bde) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1237   +/-   ##
=======================================
  Coverage   88.09%   88.09%           
=======================================
  Files           6        6           
  Lines         974      974           
=======================================
  Hits          858      858           
  Misses        116      116           
Impacted Files Coverage Δ
tools/schemacode/bidsschematools/validator.py 94.54% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Comment on lines +86 to +95
instrument:
name: Instrument
display_name: Instrument
description: |
The class of technology used to record data to a file.
For example, both EEG and iEEG are data types using biopotential amplification.
Instrument is not directly reflected in the BIDS filesystem,
though the specification itself generally nests **data type**-specific sections within
**instrument**-specific chapters.
**Data types** reflects specific applications of **instruments**.
Copy link
Member Author

Choose a reason for hiding this comment

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

This describes the new "instrument" common principle.

Comment on lines 10 to 13
eeg:
bioamp:
datatypes:
- eeg
ieeg:
datatypes:
- ieeg
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a core change- now EEG and iEEG are both considered "biopotential amplification". At some point it would be nice to reorganize the specification to reflect this, but I don't think it's necessary.

@tsalo
Copy link
Member Author

tsalo commented Aug 19, 2022

@nellh noted that OpenNeuro uses the term "Modality", and keeps EEG and iEEG separate. If we change it to "Instrument" and combine EEG and iEEG under "Biopotential Amplification", OpenNeuro would likely be impacted, and users probably want to search for either EEG or iEEG, not both.

@CPernet
Copy link
Collaborator

CPernet commented Aug 21, 2022

  1. I think add 'modality' to glossary #1141 makes the point that modality is not well defined in BIDS, and as such is a separate issue of how OpenNeuro uses it.
  2. I agree that people want to search iEEG, MEG, EEG on their own, and @robertoostenveld said so too. @nellh could rely on the datatype rather than instrument (previous modality) and everything should keep running as before
  3. I'd have approved those changes

@effigies
Copy link
Collaborator

  1. I agree that people want to search iEEG, MEG, EEG on their own, and @robertoostenveld said so too. @nellh could rely on the datatype rather than instrument (previous modality) and everything should keep running as before

Yes, OpenNeuro can maintain its own mapping. It has been nice to rely on BIDS validation outputs directly, as it reduces maintenance tasks when updating software for new BEPs and avoids getting out of sync.

I wonder if we could keep modality as an unexposed component of the validator that OpenNeuro (and other tools that want a more intuitive, albeit less logically consistent, grouping) could rely on, so that this information does stay single sourced. So it would get emitted by the validator JSON but not be exposed in UI or used in schema rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion ongoing discussion opinions wanted Please read and offer your opinion on this matter schema Issues related to the YAML schema representation of the specification. Patch version release. schema-structure Changes to the fundamental organization/structure of the YAML schema. Minor version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add 'modality' to glossary Rename modalities.yaml in schema
3 participants