BIDS 2/8: check sidecar values against the schema before writing them - #66
Merged
Conversation
Every `objects/metadata` entry in the BIDS schema is valid JSON Schema -- the reference validator feeds them to Ajv verbatim -- so a sidecar value can be checked without a dataset, a validator binary, or a network call. `lib/bids.py::value_problem` does that, registering the schema's own `objects/formats` patterns so `format` constraints are enforced rather than silently skipped. `save_json` now runs it last, on exactly what is about to be written. A value that fails moves to a `<key>Raw` key with a warning naming the field. Writing it under its BIDS name would earn a JSON_SCHEMA_VALIDATION_ERROR (severity *error*); dropping it would lose a real Bruker reading. Demoting costs neither. No schema field ends in `Raw`, so the name cannot collide. Only *values* are checked here, not which fields are required: that lives in `rules/sidecars`, whose selectors `bidsschematools` can parse but not evaluate. Field inventory stays the reference validator's job. This is stricter than the reference validator, which turns out to matter. The validator only value-checks a field named by a rule group whose selectors pass, and `RepetitionTime` is named only in `func.MRIFuncRepetitionTime` and `mrs.MRSRepetitionTime`. On the PV6 lego phantom, two RAREVTR anat sidecars were shipping `"RepetitionTime": [0.5, 1.18, 5.0]` -- an array where BIDS wants one number -- and the validator reported zero errors, because no applicable rule for an anat T2w names the field. Five more on PV5.1. They are now demoted and visible; correcting the mapping (the `InversionTime` treatment, which already guards `np.ndim(TI) == 0`) belongs to PR3. Tests cover each historical bug class -- SoftwareVersions as float, InversionTime as array, FlipAngle 0, PhaseEncodingDirection 'col_dir', the `*_bold.nii.gz` glob -- all offline, in the fast suite. Checked on PV5.1/PV6/PV7 conversions: 0 validator errors, warnings unchanged at 499, seven invalid values removed from sidecars without losing a reading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1Va9qd8WcRtcZRXJb5MLh
Two fixes to the plan as written: - The guard test was listed under PR2, but the verdict table it enforces is not populated until PR3, so it would have failed on day one. Moved to PR3. - Record why the offline value check is not redundant with the reference validator, since that was not obvious until it found something: the validator only value-checks fields a passing rule group names, and `RepetitionTime` is named only for func/bold and mrs. Seven anat sidecars across PV5.1 and PV6 were shipping it as an array with the validator reporting zero errors. Also note the resulting PR3 item: `RepetitionTime` needs the array guard `InversionTime` already has, for variable-TR (RAREVTR) scans. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1Va9qd8WcRtcZRXJb5MLh
There was a problem hiding this comment.
Pull request overview
Adds an offline, schema-driven validation step for BIDS sidecar values at write time, demoting schema-invalid entries to <key>Raw (with warnings) so converted datasets avoid validator “error” findings without losing Bruker-derived measurements.
Changes:
- Add
jsonschemaas a runtime dependency to validate sidecar values against BIDSobjects/metadata. - Implement
bids.value_problem()(including schemaformatregex enforcement) and integrate demotion logic intoBrukerLoader.save_json(). - Add offline unit tests covering historical invalid-value cases and the demotion behavior; update the conformance plan accordingly.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Adds jsonschema to the locked runtime dependency set. |
pyproject.toml |
Declares jsonschema>=4.0 as a runtime dependency for sidecar value checking. |
brkraw_legacy/lib/bids.py |
Introduces schema-based sidecar value validation with custom format checking. |
brkraw_legacy/lib/loader.py |
Adds write-time demotion of schema-invalid sidecar values to <key>Raw. |
tests/06_bids_test.py |
Adds offline tests for value validation and demotion behavior. |
BIDS_CONFORMANCE_PLAN.md |
Documents the stricter-than-validator value-check approach and rationale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+205
to
+220
|
|
||
| def value_problem(field, value): | ||
| """The first BIDS schema violation of ``value`` for ``field``, else ``None``. | ||
|
|
||
| Also ``None`` for a name the schema does not define. A sidecar may legitimately | ||
| carry non-BIDS keys, and deciding about those belongs to whoever put them there | ||
| (see the deliberate ones marked in ``lib/reference.py``) -- not to a check whose | ||
| only source of truth is the schema. | ||
| """ | ||
| definition = _SCHEMA.objects.metadata.get(field) | ||
| if definition is None: | ||
| return None | ||
| errors = sorted( | ||
| Draft202012Validator(definition, format_checker=_FORMAT_CHECKER).iter_errors(value), | ||
| key=str) | ||
| return errors[0].message if errors else None |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1.
Every
objects/metadataentry in the BIDS schema is valid JSON Schema, so a sidecar value can be checked without a dataset, a validator binary or a network call.save_jsonruns the check last, on exactly what is about to be written; a failing value moves to a<key>Rawkey with a warning.Writing it under its BIDS name would earn
JSON_SCHEMA_VALIDATION_ERROR(severity error); dropping it would lose a real Bruker reading. Demoting costs neither.This is stricter than the reference validator, and that turned out to matter. The validator only value-checks a field named by a passing rule group, and
RepetitionTimeis named only for func/bold and mrs. Seven anat sidecars across PV5.1 and PV6 were shipping it as an array from variable-TR RAREVTR scans, with the validator reporting zero errors.Tests cover each historical bug class, all offline.
🤖 Generated with Claude Code
https://claude.ai/code/session_01L1Va9qd8WcRtcZRXJb5MLh