[SCHEMA] Add metadata term files by tsalo · Pull Request #774 · bids-standard/bids-specification

tsalo · 2021-04-13T19:27:26Z

References #699. Continues from #762.

Changes proposed:

Take the metadata JSON files from the bids-validator and convert them to YAML format.
Add "description" and "key_name" fields to the metadata YAML files.
Add functions to render the metadata information in tables within the schema. These functions require the specification text to include the requirement levels.
Create files for a number of metadata terms that aren't present in the validator, but are present in specification tables.

To do:

Identify and incorporate section-specific definition modifications.
Fix rendering of multiple equivalent data types (e.g., MEGCoordinateSystem, which renders as string or string or string or string).
update w.r.t. [ENH] Add device and acquisition metadata for physio files #806
Remove hardcoded tables (after review)
deprecate ScanDate field after [FIX] Deprecate ScanDate (PET) in favor of AcquisitionTime in scans.tsv files #798 is merged (could be anytime now)
potentially update with respect to [ENH] add metadata to PET calibration factor: "DoseCalibrationFactor" #825 (if merged)

Reviewing

I think the best approach to reviewing this PR is to (1) review the proposed metadata schema structure defined in the schema README and a handful or randomly chosen example terms, and (2) compare the rendered metadata tables against the hardcoded ones in the generated specification. These tasks can be split across multiple reviewers.

* Draft a handful of metadata term files. * Add example with specific possible values. * Match the validator schemas better. * Fix formatting. * Fix formatting again! * Draft semi-functional rendering functions. * Use unit abbreviations. * Add more fields. * Get macro working. * Add terms from first table. * Add AnatomicalLandmarkCoordinateSystem. * fMRI task information table. * More terms. * More terms. * More terms. * EchoTime and FlipAngle * Add tables. * More tables. * More terms. * More terms. * More terms. * Fix spacing. * Clean things up. * More terms. * More terms! * More terms. * Some iEEG terms. * More iEEG terms. * Add ASL labeling terms. * Next batch. * More terms. * More terms. * More terms. * Fix mistakes. * Last terms. * Reference yamls. * Fix mistakes. * Change format of coordinate system files. * Use degree in associated files. * Some of the missing terms. * A few more terms. * Fix typos. * More terms. * More terms. * More terms. * More terms. * More terms. * More terms. * Last terms. * Fix link. * Fix internal links. * Fix links for real. * Derivative terms. * Fix up code link. * Use backslashes for continued strings. * Replace $ref with file contents. Also support plural datatype strings and all manner of newlines in descriptions. * Fix genetics. * Describe the structure of metadata YAML files. * Make metadatatype function recursive. * Improve search function. * Start adding PET fields. * Add some fields. * More terms. * More terms. * More terms. * Fix mistakes. * More terms. * Replace InstitutionDepartmentName with existing InstitutionalDepartmentName. * More terms. * More terms. * More terms. * More terms. * More terms. * More terms. * More terms. * Last terms. * Add unit format for strings. Unused for now but could be useful later. * Add dataset_relative and participant_relative string formats. * Update READMEs. * Fix formats in README. * Support table-specific metadata description extensions. * Employ description extensions with IntendedFor. * Remove explicit defaults from YAML files. * Replace Minimum with minimum. * Replace inclusiveMaximum with maximum. * Replace implicit links with explicit ones. * Rename key_name to name. * Rename "Unit" to "Units"

tsalo · 2021-04-13T19:29:24Z

Now that these changes are on a branch of the main repository, it should be easier for other folks to contribute directly. The main thing that I could use help with is generalizing the base metadata definitions when necessary and appending section-specific information to those base definitions in the macro calls.

src/schema/metadata/MultibandAccelerationFactor.yaml

src/schema/metadata/RepetitionTimeExcitation.yaml

src/schema/metadata/BackgroundSuppressionNumberPulses.yaml

src/schema/metadata/EpochLength.yaml

src/schema/metadata/AssociatedEmptyRoom.yaml

From #786.

Should address the "string or string or string or string" issue.

It's easier to identify as a special case.

The type is complicated, but I _think_ I've got it figured out.

src/schema/metadata/SpatialReference.yaml

tsalo · 2021-04-26T17:57:12Z

Okay, I think this is finally ready! @bids-standard/maintainers if any of you have the time, I'd appreciate your review.

effigies

Started by reviewing the code. Have not yet looked through the schema files or checked all of the rendered tables.

One thing I worry about, after moving to this, is that minor bugs in the code may result in visible changes to a table without being obviously part of the review process. Do we have a strategy to note when a change to the schema code produces a change in the generated markdown?

tools/schemacode/schema.py

@effigies

Thanks @effigies! Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>

effigies · 2021-04-27T16:21:56Z

Quick note: I ran across yaml.safe_load which has been around forever as a convenience function equivalent to partial(yaml.load, loader=yaml.SafeLoader).

tools/schemacode/schema.py

sappelhoff

review for Task (including resting state) imaging data

sappelhoff · 2021-07-05T08:46:29Z

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

+{{ MACROS___make_metadata_table(
+   {
+      "RepetitionTime": "REQUIRED",
+      "VolumeTiming": "REQUIRED",


Wouldn't it make sense to say:

Suggested change

"VolumeTiming": "REQUIRED",

"VolumeTiming": "REQUIRED, unless `RepetitionTime` and `DelayTime` are defined.",

And make similar adjustments for RepetitionTime and DelayTime requirement levels?

I notice that there are more fields that could benefit from such an overhaul (AcquisitionDuration)

Could we push this one to a separate issue/PR? I think it ties into that weird dependency table for timing-related field and will require more thought than I can put in at the moment.

ugh, you are right. Let's not open that can of worms (especially not me; EDIT: Given my lack of experience with MRI).

src/schema/metadata/NumberOfVolumesDiscardedByScanner.yaml

src/schema/metadata/NumberOfVolumesDiscardedByUser.yaml

src/04-modality-specific-files/01-magnetic-resonance-imaging-data.md

sappelhoff

finished my run through the remaining MRI sections

src/schema/metadata/CASLType.yaml

sappelhoff

I now finished going through all changes and provided that my remaining comments are addressed, I think we can merge this 👍

This is a great piece of work @tsalo, thanks a lot.

Co-authored-by: Stefan Appelhoff <stefan.appelhoff@mailbox.org>

tsalo · 2021-07-07T16:02:55Z

@Remi-Gau @effigies In yesterday's maintainers call, we decided that it would be unrealistic to request a full review from anyone else, so instead of reviewing all of the content we were wondering if you'd be willing to do more of a conceptual review or a code review. We should have plenty of time before the next release for folks to identify any issues with the content, so any small content issues can be resolved after this is merged. WDYT?

Tokazama · 2021-07-07T19:40:54Z

Is there any reason that all the metadata files are separate instead of consolidated into one file (or even just a few)?

tsalo · 2021-07-08T16:10:50Z

The biggest reason is that I find individual files easier to work with than one big file. While the idea of a few files is appealing, I don't think it would work well for metadata. The obvious delineation is based on data type, but unfortunately many metadata terms can be used for multiple data types.

Remi-Gau · 2021-07-09T07:55:40Z

@Remi-Gau @effigies In yesterday's maintainers call, we decided that it would be unrealistic to request a full review from anyone else, so instead of reviewing all of the content we were wondering if you'd be willing to do more of a conceptual review or a code review. We should have plenty of time before the next release for folks to identify any issues with the content, so any small content issues can be resolved after this is merged. WDYT?

Good with me though I will only be back "on duty" later this month (26th) so don't let my silence hold you back on the merge anyway (plus most of my comments will be on formatting and missing comas most likely).

sappelhoff

@tsalo could you please update this PR with respect to

#798
#825

Then I'd be fine with removing the hard-coded tables and merging, as Chris and Remi are currently unavailable and we didn't plan on having a "deep" review anyhow (that's already happened).

tsalo · 2021-07-13T15:28:25Z

Awesome! I incorporated the changes for the two PRs, and looked at the rendered docs. The docs look good, so I'm going to remove the hardcoded tables now.

tsalo · 2021-07-13T15:55:37Z

Okay the tables have been removed and the CI is passing.... should I merge? Is it time?!

effigies

Did a code review. Only one suggestion, really. I haven't tested, so you may want to test before merging. Or reject if it seems less clear.

tools/schemacode/schema.py

Co-authored-by: Chris Markiewicz <markiewicz@stanford.edu>

tsalo · 2021-07-13T16:41:04Z

The rendered docs look good with the new changes, so I'm merging!

effigies · 2021-07-13T16:41:17Z

Seems to work. If you're happy to merge, fire away!

sappelhoff · 2021-07-13T17:40:46Z

😎 nice work! Very happy to see this in

tsalo requested review from chrisgorgo, effigies and sappelhoff as code owners April 13, 2021 19:27

Improve make_metadata_table docstring.

2ecfc88