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

Inheritance Principle: Even more relaxed #5

Open
wants to merge 1 commit into
base: inheritance_multiple_files_per_level
Choose a base branch
from

Conversation

Lestropie
Copy link
Owner

Implements bids-standard#1195.

Proposed extension of bids-standard#1003. This PR specifically encodes the proposed change for making the IP even more powerful / general than proposed in bids-standard#1003, and does not include the changes between that PR and master.

This change would theoretically simplify the implementation of complex inheritance for BIDS APIs, as detection of applicability to any given data file would be determined on a per-metadata-file basis individually.

I do however suspect that it would be challenging to implement for the BIDS validator. Would very much appreciate feedback from anyone involved in validator development.

Additional permissibility in the Inheritance Principle.
Rather than forcing an unambiguous ordering of applicable metadata files within a single directory, instead only forbid the specific circumstance that leads to metadata content ambiguity, which is where two metadata files cannot be unambiguously sorted by either filesystem directory location or number of entities in file basename, and they contain the same metadata key.
@rwblair
Copy link

rwblair commented Sep 14, 2022

As far as the javascript validator is concerned: At first glance both the old and new schema validator will be able to implement this without tons of work. The ordering is straight forward, good work.

@Lestropie
Copy link
Owner Author

Another prospect that came up during discussion:

Rather than forbidding key conflicts only in the scenario of applicable metadata files in the same directory with the same number of entities (and therefore the order of loading of those two metadata files is ambiguous), one could instead forbid key conflicts between any two metadata files within a directory that are both applicable to any given data file, regardless of their relative number of entities.

This would forbid certain use cases, but is potentially easier to understand for some, and I was thinking that it may be easier to validate. But it sounds like this isn't actually the case. So I wouldn't personally be pushing for this alternative. Documenting here regardless just to show that it's been considered.

@effigies
Copy link

effigies commented Sep 15, 2022

Thanks for this, this is very clear. One thing that would be helpful to understand the impact is to see how tools currently handle these situations where there are multiple applicable sidecars at a level. Are there errors, or do we silently get different results across tools?

I can show for PyBIDS that breaking the BOLD images in the synthetic dataset into bold.json and task-*_bold.json where the bold.json contains the common fields, PyBIDS will just skip over bold.json. (Moving them in to the func/ directory and adding the appropriate sub/ses entities doesn't change this.)

So at least there won't be a silent conflict; there will be missing metadata. If the metadata are critical, then the tool will fail. On the other hand, if the metadata affect processing when present, then the tool will change behavior silently.

For the validator, it also seems not to find the stripped out metadata, which again will be noisy for required metadata but silent for optional/prohibited metadata.

@Remi-Gau could perhaps check on BIDS-MATLAB.


This does still leave me concerned with incompatibilities between new datasets that take advantage of this change and tools that take some time to catch up. It might be worth considering whether this would be BIDS 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants