ENH: Add overrides= parameter to read_raw_brainvision for BIDS header repair#13884
Merged
drammock merged 10 commits intomne-tools:mainfrom May 5, 2026
Merged
Conversation
… repair
Adds an opt-in dict parameter for reading non-spec-compliant .vhdr files
where the header contradicts the actual layout (e.g. renamed BIDS
siblings, missing MarkerFile=, truncated [Channel Infos]). Six keys:
data_fname, marker_fname, n_channels, sfreq, ch_names, units_fallback.
Each applied override emits a RuntimeWarning; unknown keys raise
ValueError. None / {} keep stock behavior. Mirrors the
read_raw_edf(units=...) precedent.
Closes mne-tools/mne-bids#1598.
bruAristimunha
requested review from
agramfort,
drammock,
larsoner and
sappelhoff
as code owners
Reduce 7 test functions / 18 scenarios down to 5 functions / 10
scenarios by collapsing the marker_fname split, dropping the
absolute-path parametrize on data_fname, dropping the truncation-warn
edge case, and trimming the validation parametrize. Add an explicit
test_overrides_default_unchanged that asserts overrides=None and
overrides={} produce identical (nchan, sfreq, ch_names, meas_date,
annotation count) to the no-overrides call.
Verified manually with warnings.simplefilter('error') that the default
read path emits zero warnings.
larsoner
reviewed
May 4, 2026
…logger.info Fixes test_docdict_order CI failure (key was alphabetically misplaced between brain_kwargs and brain_update). Also addresses review feedback: override warnings become logger.info messages that show original vs. override values, and the docstring uses a definition list.
Contributor
Author
|
@scott-huberty, @larsoner, @drammock, if we merge this, this will unlock many fixes on the mne-bids, and will not even need a PR in mne-bids :) btw, @larsoner, thank you for point me extra_parameter of the read at mne :) |
…all spec-required keys The override branches called cfg.get(..., key) purely to log the original header value before announcing the override; that lookup raised NoOptionError exactly in the case the override was meant to recover from. Wrap the log-only lookups (DataFile, MarkerFile, NumberOfChannels) and the required SamplingInterval lookup in _aux_hdr_info so the override actually takes effect when the header lacks the key. Extend overrides to every spec-required header key with three new options: data_orientation ([Common Infos] DataOrientation), data_format ([Common Infos] DataFormat), and binary_format ([Binary Infos] BinaryFormat). Treat a missing or empty MarkerFile= entry as "no markers" (per the BV Core Data Format spec, MarkerFile= is optional). Collapse the six per-key override+log+fallback blocks behind one _hdr_get helper, and refactor _validate_overrides into three dispatch tables (type / range / enum) plus the two genuinely irregular cases (marker_fname=False sentinel, ch_names per-element + uniqueness). Tests: parametrize test_overrides_recover_missing_header_keys across all seven file-/header-key keys; add validation cases for the three new enum keys; add test_empty_marker_file_means_no_markers for the spec-compliant empty-MarkerFile= path.
The previous fix tolerated a missing SamplingInterval= header by parsing NaN inside _aux_hdr_info and re-raising NoOptionError from _get_hdr_info when no sfreq override was supplied. Replace that sentinel dance with an explicit sfreq_override parameter on _aux_hdr_info: when set, skip the cfg lookup entirely; when not set, the natural NoOptionError surfaces. The second _aux_hdr_info call (impedance parsing in __init__) reuses the resolved info['sfreq'] so it never re-parses the missing key.
larsoner
approved these changes
May 5, 2026
Member
larsoner
left a comment
larsoner
left a comment
There was a problem hiding this comment.
Okay for me and failures are unrelated. @scott-huberty do you want to look?
Contributor
scott-huberty
left a comment
scott-huberty
left a comment
There was a problem hiding this comment.
Kind of a big diff so I did not review everything. and I am not very familiar with this module, but I have left a few comments!
- Defensive sfreq extraction: catch NoSectionError/NoOptionError in _aux_hdr_info; raise an informative RuntimeError when neither cfg nor overrides['sfreq'] supply a value. - Richer override log: report original cfg sfreq alongside override value (or note that the header was unparseable). - Flatten _OVERRIDES_VALID_KEYS into a single set. - Replace overrides=None round-trip test with an inspect.signature default check; keep the empty-dict equivalence test as its own case.
drammock
approved these changes
May 5, 2026
Comment on lines
+733
to
+735
| """``overrides={}`` matches the no-overrides read exactly.""" | ||
| baseline = read_raw_brainvision(vhdr_path) | ||
| raw = read_raw_brainvision(vhdr_path, overrides={}) |
Member
There was a problem hiding this comment.
this is fine as-is, but just want to call attention to mne.utils.object_diff which should be able to diff Raw files. Might be overkill here (vs. 5 assert statements) but can be nice if you want to compare more attrs
When the MarkerFile= path declared in a .vhdr does not exist, fall back to the co-located <stem>.vmrk and warn; if no sibling exists, warn and read without annotations instead of raising FileNotFoundError.
auto-merge was automatically disabled
May 5, 2026 21:44
Head branch was pushed to by a user without write access
Contributor
Author
|
hey @drammock, @scott-huberty and @larsoner! I am so sorry for my last commit. I was just planning to do another PR, but then by accident here, but as was just 5 line, if you can forgive, and review this very small detail, i think i am done with PR at mne-python about the brainvsion. |
drammock
approved these changes
May 5, 2026
larsoner
added a commit
to larsoner/mne-python
that referenced
this pull request
May 6, 2026
* upstream/main: ENH: Add overrides= parameter to read_raw_brainvision for BIDS header repair (mne-tools#13884)
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
4 participants
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.
Adds opt-in
overrides=dict | Nonetoread_raw_brainvisionfor non-spec.vhdrfiles. Closes mne-tools/mne-bids#1598. Mirrorsread_raw_edf(units=...)(#11099).Nine keys cover failure modes with no escape hatch on
main:mainerrorFileNotFoundError(renamed.eeg)data_fnameNoOptionError/FileNotFoundError(missingMarkerFile=or.vmrk)marker_fname(path orFalse)NoOptionErroronNumberOfChannels=/SamplingInterval=/DataOrientation=/DataFormat=/BinaryFormat=n_channels,sfreq,data_orientation,data_format,binary_formatRuntimeError("Missing channel units")(truncated[Channel Infos])units_fallbackch_namesEach applied override is logged at INFO;
None/{}preserves current behavior.Spec note
The nine keys cover every spec-required header field per the BrainVision Core Data Format 1.0 spec (Brain Products GmbH, Document Version 2.5, 2019-12-12).
One default-behavior change is intentional and brings the reader closer to the spec it claims to read: a header without
MarkerFile=(or with an empty value) now reads with no annotations instead of raisingNoOptionError. From §1 General Statements:i.e., on
mainthe reader rejects spec-compliant headers. The override-supplied path is unaffected;marker_fname=Falsestill works.Tests
53 brainvision tests pass: pre-existing coverage + table-driven override validation, a 7-case parametrized missing-header-key recovery (one per spec-required key), default-unchanged guard, file-path / units-fallback / empty-
MarkerFile=cases.Affected dataset list in mne-tools/mne-bids#1519; open design questions in mne-tools/mne-bids#1598.
cc @cbrnr @sappelhoff @hoechenberger @drammock @larsoner