[ENH] Inheritance Principle: Advise against value overloading. Plan to deprecate support for overloading for BIDS 2.0#1834
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1834 +/- ##
=======================================
Coverage 88.04% 88.04%
=======================================
Files 16 16
Lines 1380 1380
=======================================
Hits 1215 1215
Misses 165 165 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Seems reasonable to me. If there's consensus here, it would be ideal to raise a validator warning and to provide a tool to remove overrides from datasets automatically, with validation being the more important. |
Good idea; added to my list. |
|
Tagging as needs implementation. Recommendations without validator warnings will have essentially no impact. |
|
I created bids-standard/bids-validator#117 to track implementation. I think this would be a good first issue for a new contributor to the validator. |
| the Principle defines the *order of precedence* of such metadata content; | ||
| this is necessary for resolution of conflicts if the same metadata field | ||
| contains different values in different metadata files | ||
| (though it is RECOMMENDED to avoid such overloading). |
There was a problem hiding this comment.
since in BIDS 2.0 we make it not allowed (ERROR), we could also relax (completely remove?) the order of precedence statement since it would not matter, right? or should we leave it in to disambiguate implementations robust operation and say that e.g. top level (most common) definition takes precedence?
There was a problem hiding this comment.
The mechanisms relating to determination of the order of metadata files remains relevant, not for .json but for other metadata extensions. For formats where it is only the "nearest" metadata file that applies, there has to be no ambiguity about which is "nearest".
There was a problem hiding this comment.
well, while it is just a WARNING it remains relevant even for JSONs . Even when it would be ERROR, it would be nice to describe expected behavior in case of non-compliance, but I think it would not be possible if we relax requirements and allow multiple files at the same level to provide it.
yarikoptic
left a comment
yarikoptic
left a comment
There was a problem hiding this comment.
Small change for a human, big leap for humanity!
It would have been nice to
- assess the impact of such a change on existing datasets (openneuro). I guess the easiest way is to just run validator and see how many hit that new WARNING
- provide "upgrade" functionality (#1984) which would address the warning (help to move metadata down the hierarchy if encountered).
yarikoptic
changed the title
|
I adjusted title to provide more context when used in CHANGELOG. Please review @Lestropie @effigies |
effigies
left a comment
effigies
left a comment
There was a problem hiding this comment.
LGTM. @yarikoptic Does your latest comment require a change to this text now, or is it more tagged for BIDS 2.0? I think the order of precedence statement should not be removed until it's an error to conflict.
|
Implementation: bids-standard/bids-validator#214 |
ohbm/hackathon2025#13 if successful would provide a more tailored tool for detecting these. |
|
Cc @ericearl |
effigies
changed the title
- Add migrate.py module with base migration framework including Migration class, MigrationRegistry, and decorators - Add migrations.py with three specific migrations: - standardize_generatedby: Convert pre-standard provenance to GeneratedBy (BEP028) - fix_inheritance_overloading: Check for deprecated inheritance overloading patterns (PR #1834) - fix_tsv_entity_prefix: Check for missing entity prefixes in TSV files (PR #2281) - Add CLI commands: 'bst migrate list', 'bst migrate run', 'bst migrate all' - Add comprehensive tests for migration framework and specific migrations - All tests passing (24 tests total) Co-authored-by: yarikoptic <39889+yarikoptic@users.noreply.github.com>
4 participants
Re-commit of b7a3ab6 that was rejected as a late addition to #1807; re-posting here for separate review as suggested.
I've become progressively convinced that the ability to overload metadata fields via Inheritance is a major source of the disparity in opinions about the Inheritance Principle. Regardless of whether you like or hate it, where I think there should be consensus is that overloading should not be intentionally used, even if BIDS 1.x explicitly permits it and it was used as one of the key demonstrative examples.
The proposed change hopefully also helps to contextualise why the subsequent text is a complex but necessary set of rules.