[SCHEMA] Render schema elements in text by tsalo · Pull Request #610 · bids-standard/bids-specification

tsalo · 2020-09-16T21:53:31Z

References #544 and closes #591. Uses the mkdocs-macros plugin to generate certain markdown elements (e.g., the filename patterns) from the schema. This should easily extend to tables and json examples, as well.

Changes proposed:

Replace manual Entity Table and Entity description file creation scripts with functions that are called by the mkdocs-macros plugin within the markdown files.
Move all auxdatatypes into new groups under the associated data types. This is primarily because I had so much trouble getting a generic filtering function working with the original structure.
Add standard Python stuff to the gitignore.
Reorganize Python code into a mini package.
Add several Python libraries and macros as dependencies.
Replace entities in datatypes files with the actual names instead of their shorthand versions.
Split the iEEG/EEG/MEG entity table into a Biopotential Amplification (iEEG/EEG) table and an MEG table.

TODO:

Reorganize Python code. Even though the macros documentation says that the define_env function can be in a module, I can't seem to get that working, so it's in its own file at the top level for now.
Generic filtering function. We need to be able to filter the schema down to specific classes or elements within classes.
- Something on the order of pybids is probably overkill, but I imagine the pybids devs would have some insights on this.
- I added something in 9ce6d7f that is fairly flexible. We should improve it at some point though.
Updated entity tables. Currently, because of the lack of a filtering function, only the full entity table is generated.
Combine .nii and .nii.gz as .nii[.gz] in file formats.
~~Consider dropping .json from the list of extensions in the schema. JSON files are not alternatives to the other extensions. They are required (at some level).~~
- I've decided to ignore this nuance for now. Will just treat json like any other extension and show it in the format snippets.
Deal with remark. TravisCI is failing because remark flags the code snippets like {{ make_filename_template(datatypes=["anat"]) }} as bad.
Update contributing information. Fewer steps are needed when updating the schema now, since rendering happens automatically.
Fix PDF rendering. Currently, it just shows the inline code without the text produced by the code.
Remove old outputs (currently retained for comparison).

More To Dos (added by @sappelhoff here for better visibility, but can be done by anybody)

open issues:
- eventually turn schemacode into its own package and migrate to separate repo?
  - I've brought this up in [INFRA]: post-#475 - migrate schema access code (Python API) to bids-schema #543.
- add python linter and style check to CI for pdf build code, tools, schemacode
  - I've brought this up in [INFRA]: post-#475 - migrate schema access code (Python API) to bids-schema #543.
- add specific license for code in this repo ... CC BY 4.0 is more for the spec itself, not so adequate for the code
- once this is merged, we'll want to generate "templates" also for files like README, dataset_description.json, and so on (currently not done)
- issue to track that files like "scans.tsv/json" and "phenotype/*" get an auto-generated "template" section as well
- issue for pdf build so that markdown table rows are distinguished with white and gray "fills"
  - Opened Alternating row fills in tables within the PDF build #659.

tsalo · 2020-09-17T18:36:05Z

@yarikoptic check this out! Here is an example filename format example snippet. The first one is autogenerated from the specification, while the second one is the preexisting one. It's far from perfect, but I think it's pretty promising!

auxdatatypes are still difficult to work with.

Still far from perfect.

tsalo · 2020-10-02T18:32:41Z

I'm marking this ready for review, with a couple of caveats:

I still need to update the remark settings so that it will play nicely with macros.
Some of the filename format snippets for data types with many suffixes look.... not the best. Unfortunately, cleanly combining them without also combining out the json files will require either (1) hacky hardcoding or (2) conditional logic implemented in the schema (see Mutual exclusion and conditional relationships in the schema #620).
What are MEG _markers files? They're in the schema but I don't know where they came from.
I've kept the original versions of the rendered elements in the specification for now to make comparison easier.

sappelhoff · 2020-10-02T19:51:41Z

What are MEG _markers files? They're in the schema but I don't know where they came from.

😲 did you find a spot in the spec that yields more information?

I just found this in https://bids-specification.readthedocs.io/en/latest/99-appendices/06-meg-file-formats.html#kityokogawaricoh

Seems like a badly defined suffix ... reminiscent of the part -> split situation we had a while ago, where an MEG suffix was only defined in an appendix.

tsalo · 2020-10-02T20:01:01Z

It looks like _markers has been in the appendix for a couple of years (first mention I could find is #28), but I can't find uses outside of the appendix. I'm not sure where to go from here, except maybe to open a separate issue and tag some MEG folks?

sappelhoff · 2020-10-02T20:02:58Z

I'm not sure where to go from here, except maybe to open a separate issue and tag some MEG folks?

yes please 👍

.remarkrc

yarikoptic · 2020-10-08T17:36:05Z

@tsalo Sorry for being slow... The example snippet and rendering look absolutely wonderful to me!!! Awesome job!
The beauty of having it auto-generated is that at some point if we decide to change "rendering" - we would just do that in a single spot -- code. E.g., as number of entities grows, we might want to us e.g. sub-<label>[_ses-<label>]_task-<label>[_entity-<value>]*_bold.json with some "tooltip" when in HTML so when hovering over [_entity-<value>]* listing all the entities ... (not here and not now, but in principle, just as an idea).

sappelhoff · 2021-01-04T09:08:00Z

Just a reminder to us that we should merge this after ASL and qMRI, but before PET ... so that we can finally start reaping the benefits. see also #690 (comment)

src/schema/datatypes/eeg.yaml

sappelhoff

apart from one comment, LGTM

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

sappelhoff · 2021-01-29T08:36:59Z

@effigies @emdupre @Remi-Gau @yarikoptic we would like to merge this PR but would feel more comfortable with at least one more review.

EDIT: To clarify: Perhaps the most valuable review would be to open the build artifact (pdf or HTML), and compare the new computer rendered template with the old, hand-written template. Currently both are left in this PR for comparison --> the upper one is the new one, the lower one the old one.

So you can basically go through the artifact, searching for "template" and then do a check if the entities, suffixes, etc. match and sound about right.

example:

yarikoptic

I have skimmed through it, saw some of my requested (forgot the medium we communicated through with @tsalo awhile back) changes reflected . This review includes some of my comments (e.g. eventually should be RFed so there is no sys.path) which I never submitted (will not remove them) but they can be ignored.
This is a HUGE work and progress, thank you @tsalo for leading it, and everyone who contributed. Let's get it merged and work on top of that ;)

yarikoptic · 2020-11-20T22:25:41Z

CONTRIBUTING.md

-```bash
-python tools/bids_schema.py entity src/schema/ src/99-appendices/04-entity-table.md
-```
+As such, you need to ensure that the functions used throughout the specific to render these elements are appropriately referencing the schema; in essence, that, if your changes do impact how functions should be called, the function calls have been updated.


Reads odd, needs tune up and probably split into two sentences

yarikoptic · 2020-11-20T22:26:59Z

CONTRIBUTING.md

+
+Run `mkdocs serve` and open `localhost:8000` to browse the rendered specification.
+Make sure that all filename format templates, entity tables, and entity definitions are correct
+and that the code that generates these elements is not broken by your changes.


In principle, pr would do that right?

For comparing pdf changes, pdfdiff could be handy

You're right, the CI will render the files, but the CI won't fail if the code fails.

oh, how come is that? because it is too deep (invoked through remake etc) and something just swallows its failures? Ideally we should fail if anything fails in the code... if not possible "legitimately", could be done by wrapping __main__ invocation in try/except and then creating some sentinel file and then checking after invocation of the outside tool that there is no such file.

just a comment/idea: should not stop this PR from being merged!

I think it's a problem with the mkdocs-macros plugin.

yarikoptic · 2020-11-20T22:29:01Z

main.py

+specification and are run/rendered with the mkdocs plugin "macros".
+"""
+import sys
+sys.path.append("tools/")


if you do this, should take for of __file__ and construct full path so it wouldn't rely on being invoked from the specific directory

But it isn't even a script - not yet sure but better ways could emerge through review

Yep, make it tools/Init. Py

yarikoptic · 2020-11-20T22:32:53Z

pdf_build_src/process_markdowns.py


 import numpy as np

+sys.path.append("../tools/")


Another one

It looks like adding __init__.py broke things.

@tsalo I suggest to revert for now. We can smooth and streamline later on.

yarikoptic · 2021-01-29T17:31:13Z

Note that before merge the "Remove old outputs (currently retained for comparison)." should still be done (in entities table).

tsalo · 2021-01-29T17:49:23Z

tools/mkdocs_macros_bidsschema/main.py

+import os
+import sys
+
+schemacode_path = os.path.abspath(os.path.join(os.path.dirname(__file__), ".."))


Sorry @yarikoptic, but this is the current version of what you commented on in #610 (comment). Should I add __init__.py to this line?

sappelhoff

as discussed in maintainers meeting: let's remove the "old template texts" and then merge

tsalo added 5 commits September 16, 2020 17:51

Use macros for filenames.

2d473fb

Update requirements.txt

b9f3ed6

Reorganize Python code.

9c7e68e

Further reorganization.

b0a935f

Get macros working.

dc75c75

tsalo changed the title ~~[INFRA] Render filename patterns in specification~~ [INFRA] Render specification elements in text Sep 17, 2020

tsalo added 4 commits September 17, 2020 16:52

Get entity tables and definitions working.

9ce6d7f

auxdatatypes are still difficult to work with.

Add tabulate to reqs.

b0d826d

Fix up filter a bit.

54a0800

Still far from perfect.

Move schemacode into tools/

2fc6d4b

tsalo changed the title ~~[INFRA] Render specification elements in text~~ [INFRA] Render schema elements in text Sep 18, 2020

Fix path.

3a81858

tsalo mentioned this pull request Sep 28, 2020

[SCHEMA] Reorganize schema #609

Closed

tsalo added 4 commits October 2, 2020 13:02

Merge branch 'master' into enh/render-schema

d6d9213

Fix up files w.r.t. recent entity name-changes.

8c9e8de

Remove auxdatatypes.

9b8575c

Update CONTRIBUTING.md

e1c3f7f

tsalo marked this pull request as ready for review October 2, 2020 18:29

tsalo requested review from chrisgorgo and sappelhoff as code owners October 2, 2020 18:29

tsalo requested a review from yarikoptic October 2, 2020 18:33

Try ignoring the specific warnings.

1871257

tsalo mentioned this pull request Oct 2, 2020

markers suffix for MEG only referenced in Appendix VI #638

Closed

tsalo commented Oct 3, 2020

View reviewed changes

.remarkrc Outdated Show resolved Hide resolved

tsalo added 4 commits January 13, 2021 15:38

Merge branch 'master' into enh/render-schema

3249291

Fix bugs in schema.

e990848

Merge branch 'master' into enh/render-schema

1548384

Update for ASL.

7bad87f

tsalo added this to the 1.5.0 milestone Jan 21, 2021

sappelhoff reviewed Jan 27, 2021

View reviewed changes

src/schema/datatypes/eeg.yaml Outdated Show resolved Hide resolved

sappelhoff approved these changes Jan 27, 2021

View reviewed changes

tsalo and others added 2 commits January 27, 2021 11:43

Update src/schema/datatypes/eeg.yaml

fcf092c

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

Fix spacing.

cb19df3

sappelhoff requested review from Remi-Gau and emdupre January 29, 2021 08:37

yarikoptic approved these changes Jan 29, 2021

View reviewed changes

tsalo commented Jan 29, 2021

View reviewed changes

tsalo added 5 commits January 29, 2021 12:55

Address @yarikoptic's point about wording.

a23aa09

Point to file instead of folder.

4b2fcae

Expand on the need to manually check rendered text.

cce5e58

Revert init change.

b45d99e

Remove hardcoded versions of auto-generated text.

1d5c687

sappelhoff approved these changes Feb 2, 2021

View reviewed changes

tsalo merged commit 644ebd1 into bids-standard:master Feb 2, 2021

bids-maintenance added a commit that referenced this pull request Feb 2, 2021

[DOC] Auto-generate changelog entry for PR #610

b4f0c2e

This was referenced Feb 2, 2021

[ENH] Clarify run indexing information for MRI acquisitions #719

Merged

[FIX] Correct entity names in YAML files #720

Merged

tsalo mentioned this pull request Feb 24, 2021

[FIX] schema for i/eeg coordsys+elecs: sub-ses-acq-space are allowed entities #743

Merged

sappelhoff mentioned this pull request Feb 24, 2021

Simplify interaction with BIDS schema auto build script #696

Closed

tsalo added schema-code Updates or changes to the code used to parse, filter, and render the schema. schema-structure Changes to the fundamental organization/structure of the YAML schema. Minor version release. labels Apr 11, 2022

Conversation

tsalo commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

More To Dos (added by @sappelhoff here for better visibility, but can be done by anybody)

Uh oh!

tsalo commented Sep 17, 2020

Uh oh!

tsalo commented Oct 2, 2020

Uh oh!

sappelhoff commented Oct 2, 2020

Uh oh!

tsalo commented Oct 2, 2020

Uh oh!

sappelhoff commented Oct 2, 2020

Uh oh!

Uh oh!

yarikoptic commented Oct 8, 2020

Uh oh!

sappelhoff commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

sappelhoff commented Jan 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yarikoptic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yarikoptic commented Jan 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sappelhoff left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tsalo commented Sep 16, 2020 •

edited

Loading

sappelhoff commented Jan 4, 2021 •

edited

Loading

sappelhoff commented Jan 29, 2021 •

edited

Loading

yarikoptic left a comment •

edited

Loading