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

doc: Document the validation model, context and inheritance principle #94

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

effigies
Copy link
Contributor

@effigies effigies commented Nov 8, 2024

This PR adds discussion of the validation model, intended to be along the lines of the "Explanations" quadrant of diataxis.

I would be interested in feedback on whether this is understandable. I move back and forth between Python for algorithm, typescript for type definition and YAML for data description. They feel natural to me, but I've been up to the eyeballs in this stuff.

@yarikoptic @bendhouseart Could I trouble you for reviews?

Rendered docs: https://bids-validator--94.org.readthedocs.build/en/94/validation-model/index.html

@effigies effigies force-pushed the doc/validation-model branch from 643ae92 to b3159f8 Compare November 8, 2024 16:46
@yarikoptic yarikoptic self-requested a review November 8, 2024 21:04
@effigies effigies changed the base branch from main to stable November 8, 2024 21:30
@yarikoptic
Copy link
Contributor

Thanks for writing it up @effigies ! In my state of mind of bids-standard/bids-2-devel#54 -- should I provide feedback as if this validation model already formulated (that is what we have in deno validator) or having being worked on and thus having a potential for renamings/restructuring?

@effigies
Copy link
Contributor Author

effigies commented Nov 8, 2024

I would prefer just on the current model. If/when we restructure, we can update the explanation.

Copy link
Contributor

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Left minor comments/questions. Informative!

docs/validation-model/index.md Outdated Show resolved Hide resolved
docs/validation-model/context.md Outdated Show resolved Hide resolved
docs/validation-model/context.md Show resolved Hide resolved
docs/validation-model/context.md Show resolved Hide resolved
fileTree = fileTree.parent
```

Using this basis, `loadSidecar` is simply:
Copy link
Contributor

Choose a reason for hiding this comment

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

where in validator we would apply inheritance's walkBack to .tsv files? e.g. if there is top level sessions.tsv and then sub-01/sub-01_sessions.tsv ?

Copy link
Contributor Author

@effigies effigies Nov 11, 2024

Choose a reason for hiding this comment

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

BIDS does not have this concept yet, so the validator does not have it. We would need to define it to know whether walkBack is even applicable (augment sub-01_sessions.tsv with sessions.tsv), or if we need to construct one global table (augment sessions.tsv with sub-*_sessions.tsv).

Copy link
Contributor

Choose a reason for hiding this comment

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

To what .tsv files we have that principle applied so far ?
We do list not only .json but also .bvec and .tsv files in https://github.com/bids-standard/bids-specification/blob/master/src/common-principles.md#the-inheritance-principle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, those are the associated files, see a couple lines down from here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I do not see any kind of "inheritance" operation there though like here | sidecar -- there it just returns first found instead of "building up" the value.

Copy link
Contributor Author

@effigies effigies Nov 13, 2024

Choose a reason for hiding this comment

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

From https://bids-specification.readthedocs.io/en/stable/common-principles.html#rules:

For tabular files and other simple metadata files (for instance, bvec / bval files for diffusion MRI), accessing metadata associated with a data file MUST consider only the applicable file that is lowest in the filesystem hierarchy.

@effigies effigies merged commit f41eb71 into bids-standard:stable Nov 11, 2024
1 check passed
@effigies effigies deleted the doc/validation-model branch November 11, 2024 16:04
yarikoptic added a commit to yarikoptic/bids-specification that referenced this pull request Nov 11, 2024
Current names duplicate their "domain" (subject_ or session_), inconsistent in
plurality (_id although for all ids), and not clear really what
'phenotype' corresponds to without reading the description.

With proposed change there is no duplication of the domain, consistency
in plurality (albeit 'id' is an abbreviation so 'ids' is a non-word), and
IMHO clearer meaning in `ids_phenotype`.

This would also allow for generalization across other entities in a perspective
bids-standard/bids-2-devel#54 - where then any entity
with folders for its level could have `dirs`. Also it could come handy to
determine `ids` for some other entities in tests.

Ref: bids-standard/bids-validator#94 (comment)

TODOs
- [ ] introduce corresponding changes to bids-validator
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.

2 participants