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

[WIP] SCHEMA bep011 #1086

Closed
wants to merge 8 commits into from
Closed

Conversation

rwblair
Copy link
Member

@rwblair rwblair commented Apr 21, 2022

A potential inconsistency I noticed when implementing bep011 #518 :
objects/columns.yaml entries have a key that is used internally to reference the entry, then a name that is what shows up in files.
objects/entities.yaml entries have a key that is used internally to reference the entry, a name field for pretty printing, and then an entity field which is what actually shows up in filenames.
objects/suffixes.yaml entries have a key that is both used internally and is what shows up in the filenames.

I have started transitioning derivatives selectors and checks to be more valid JS than our current expression language to forward the validator progress until an expression language is settled on.

This can't validate morph.tsv column names with the suggested [-][-] pattern.

There are many derivative specific columsn/entities/suffixes here, do we want to separate these in the objects directory from non-derivative bids entries?

Entries in objects/suffixes.yaml are in alphabetical order, should this order be case sensitive?

Also contains fixes for other derivatives rules using desc instead of description

@rwblair rwblair requested a review from tsalo as a code owner April 21, 2022 16:15
@rwblair rwblair changed the title SCHEMA bep011 [WIP] SCHEMA bep011 Apr 21, 2022
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.50%. Comparing base (f8cc27e) to head (98d0fa4).

Additional details and impacted files
@@              Coverage Diff               @@
##           schema-sprint    #1086   +/-   ##
==============================================
  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.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Minor nitpicks but this makes sense to me. (I haven't compared directly with text of BEP011, though.)

src/schema/objects/suffixes.yaml Outdated Show resolved Hide resolved
src/schema/objects/suffixes.yaml Outdated Show resolved Hide resolved
@tsalo
Copy link
Member

tsalo commented Apr 21, 2022

objects/columns.yaml entries have a key that is used internally to reference the entry, then a name that is what shows up in files.

I think this (and metadata) is the one we should base others off of. There's no guarantee that elements (whether suffix, metadata field, or column name) will have singular definitions, so we may need multiple entries for a given element with unique keys.

objects/suffixes.yaml entries have a key that is both used internally and is what shows up in the filenames.

I think this one should be changed. We currently have unique suffixes (the same suffix always has the same description), so we use the actual suffix as the key, but this isn't the case for columns and metadata. To make it consistent with metadata/columns, then we should use name for actual suffix, and the key would be the suffix, with the potential for __[unique_tag] to distinguish entries with the same suffix, but different descriptions.

objects/entities.yaml entries have a key that is used internally to reference the entry, a name field for pretty printing, and then an entity field which is what actually shows up in filenames.

Yeah... entities is a special case where we need some field with the "code name" (see #585 and #616). We ended up using a format that isn't very consistent with the rest of the schema. Some folks have raised concerns with the current structure, so it may be worth revisiting.

@tsalo
Copy link
Member

tsalo commented Apr 21, 2022

Proposal from today's schema call:

# suffix
TwoPE:  # some unambiguous reference that works for MATLAB and ancpBIDS
  name: 2PE  # The actual value used in the specification
  display_name: 2-photon excitation microscopy  # The pretty name
  description: |
    2-photon excitation microscopy imaging data

# entity
inversion:  # also used for variable names in pybids
  name: inv
  display_name: Inversion Time
  description: |
    If files belonging to an entity-linked file collection are acquired at different
    inversion times, the `_inv-<index>` key/value pair MUST be used to distinguish
    individual files.
    This entity represents the `"InversionTime` metadata field. Please note that the `<index>`
    denotes the number/index (in the form of a nonnegative integer), not the `"InversionTime"`
    value which needs to be stored in the field `"InversionTime"` of the separate JSON file.
  type: string
  format: index

# column
type__electrodes:
  name: type
  display_name: Electrode Type
  description: |
    Type of the electrode (for example, cup, ring, clip-on, wire, needle).
  type: string

We need to decide between name, key, value, and some other ideas for the actual string that appears in the specification.

@effigies effigies deleted the branch bids-standard:schema-sprint April 28, 2022 19:47
@effigies effigies closed this Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants