Speed up EDF and BDF reading - #14237
Merged
Merged
Conversation
bruAristimunha
requested review from
agramfort,
drammock,
larsoner,
mscheltienne and
sappelhoff
as code owners
August 28, 2026 11:36
The stride decoder reshaped a whole data record into an (n_channels, max_samp) matrix, which requires every channel to share a sampling rate. An EDF+ annotation channel usually does not: for a 64-channel 256 Hz file with a 60-sample TAL channel a record holds 16444 values while the reshape needs 16640, so 'stride_layout' was None and every EDF+ read fell back to the per-channel loop. Gate on the *selected* channels instead, and when a record is not rectangular gather each picked channel by its own offset. Files whose records are rectangular keep the existing reshape unchanged. Fast-path coverage across the EDF/BDF corpus goes from 6/22 files to 18/22. EDF+ windowed reads 1.76x, EDF+ preload 1.28x, BDF+ windowed reads 1.42x; files without an annotation channel are unaffected. Output stays bit-identical to main: 73/73 array snapshots and 52/52 annotation snapshots across 26 files.
# Conflicts: # mne/io/base.py # mne/io/tests/test_raw.py
Four review passes (reuse, simplification, efficiency, altitude) against the shape of mne-tools#14216. Measured rather than assumed: - byte-budget heuristic: never fires. n_per already caps a chunk near 10 MiB of source bytes, so temporaries stay under 50 MB of the 64 MB cap even for an adversarial 2-channel/6-hour file with reversed picks. - n_read == 1 branch: 0/200 hits on the windowed benchmark, and the general branch produces the same values. - threading: real (13-22% on preload) but reachable only via direct_output, and it is one of only two ThreadPoolExecutor sites in MNE. mne.parallel already has parallel_func(prefer='threads'). Deferred to its own PR with a benchmark. - direct_output: kept, it measures 27-28% on full preload. _read_segments_file mmap/threading and the BrainVision block sizing are reverted to main and move to a follow-up: no MNE fixture is large enough to reach the 64 MB threading threshold (largest .eeg is 3.7 MB), so it was untested at any realistic scale. Tests 549 -> 156 lines: four near-identical stride tests merged into one parametrized test, redundant file_kind axes dropped, and a test asserting x * 1.0 == x bit-exactly removed. +884/-29 -> +330/-18. Output stays bit-identical to main: 77/77 array snapshots and 56/56 annotation snapshots across 28 files.
bruAristimunha
force-pushed
the
perf/raw-io-cold-path
branch
from
August 28, 2026 12:52
9fbdbf9 to
049ff5b
Compare
The decode lived in a 118-line _read_uniform_segment that duplicated the chunk loop, a _calibrate_uniform_edf helper, and a stride_layout tuple cached in the header and unpacked far away, gated by a rectangular -> sel_in_physical_order -> in_physical_order -> direct_output chain. A reviewer had to hold two copies of the same loop in their head. It is now one branch inside the loop that was already there: gather the picked channels by their record offsets, calibrate the block straight into 'data', and mask any stim rows. No new helper functions, no precomputed tuple, no flag chain. edf.py goes from 235 to 64 added lines and is measurably faster -- EDF preload 1.26x -> 1.40x and EDF+ preload from parity to 1.47x, because every case now writes into 'data' directly, not just the aligned one that direct_output required. The tests use the two hooks the reader already has instead of recorder classes: an identity projector routes the same values through the per-channel loop, and counting _mult_cal_one tells the two paths apart. 308 -> 81 added lines, and mutation testing catches 15/15 injected defects where the longer version caught 11. Output stays bit-identical to main: 77/77 array snapshots and 56/56 annotation snapshots across 28 files.
bruAristimunha
changed the title
larsoner
reviewed
Aug 28, 2026
larsoner
left a comment
larsoner
left a comment
Member
There was a problem hiding this comment.
Just two tiny maintainability things, otherwise LGTM!
| raise RuntimeError( | ||
| f"Only {raw.size} of {expected} requested BDF bytes could be read" | ||
| ) | ||
| ch_data = np.empty(samp, dtype=INT32) |
Member
There was a problem hiding this comment.
For optimizations like this that are maybe not as readable, I like to leave in a comment the more readable three--liner that would do the same thing. It helps the conceptual model for the next person who has to debug the code (and they can easily switch to using it if they suspect the optimized code is the problem)
Comment on lines
+108
to
+110
| def test_preload_does_not_materialize_times(monkeypatch): | ||
| """Test preloading does not construct the full time vector.""" | ||
| monkeypatch.setattr("mne.io.base._arange_div", _fail_if_times_materialized) |
Member
There was a problem hiding this comment.
Now that there is this monkeypatch a few places, I think maybe we should make a fail_if_times_materialized pytest fixture that does monkeypatch / yield (DRY the code)
github-project-automation
Bot
moved this to Ideas
in 2026 MNE-Python Maintainers Sprint
Two maintainability points from @larsoner: - The BDF int24 unpack now carries the equivalent three-liner in a comment, so the next person has a conceptual model and something to swap in if they suspect the optimized version. Verified equivalent across sizes 1-100000 and at the range boundaries; the readable form is ~3x slower. - _fail_if_times_materialized becomes a fail_if_times_materialized fixture that patches and yields, used by both tests. The annotations test now has the patch active during the read too, so it is slightly stricter than before.
vulture does not model pytest fixtures: the fixture function looks uncalled and the two test parameters that request it look like unused variables. Same treatment as the other fixture names already in the allowlist.
github-project-automation
Bot
moved this from Ideas
to Done
in 2026 MNE-Python Maintainers Sprint
Member
|
Thanks @bruAristimunha ! |
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
2 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.
Reference issue (if any)
None. Supersedes #14215 (and the closed #14214); follow-up to the merged #14212 / #14213.
What does this implement/fix?
Speeds up EDF/BDF reading — first read and full preload — with no new
dependency and no API change.
over channels, including EDF+ files whose annotation channel is stored at its
own rate
<u4view instead of athree-term shift/add with a boolean sign fixup
Rawsetup no longer materializes the fulltimesvector just to computescalar bounds
.setfor annotations
Output is bit-identical to
mainin all cases: 77/77 array snapshots and 56/56annotation snapshots over 28 EDF/BDF/GDF files, compared with
np.array_equal.64 channels at 256 Hz for 10 minutes, warm page cache, medians of 5 interleaved
A/B rounds against a pristine
mainworktree on the same machine. "EDF+"/"BDF+"are the same files with an
EDF Annotationschannel at its own rate:Why EDF+ matters here
An EDF+ annotation channel is normally stored at a different rate from the
signal channels, so a data record is not an
(n_channels, max_samp)matrix andcannot simply be reshaped — a 64-channel 256 Hz record with a 60-sample TAL
channel holds 16444 values where the reshape needs 16640. Gating on all
channels sharing a rate would exclude those files, which is most clinical EDF.
Gating on the selected channels and gathering each picked channel by its own
offset covers them: fast-path coverage over the EDF/BDF test corpus is 18/22
files, the remaining four being genuinely mixed-rate reads that must resample.
Shape of the change
The decode is one branch inside the chunk loop
_read_segment_filealreadyhad — gather the picked channels by their record offsets, calibrate the block
straight into
data, mask any stim rows — so there is no second copy of theread loop to keep in sync, no helper functions, and no precomputed layout
state.
mne/io/edf/edf.pyis +64/-11.The tests use hooks the reader already provides rather than recorder classes:
an identity projector routes the same values through the per-channel loop, and
counting
_mult_cal_one(which only the loop calls) tells the two paths apart.Mutation testing catches 15/15 injected defects — wrong pick order, dropped
calibration terms, off-by-one record trimming, a skipped stim mask, a gate that
ignores picks, and reassociated arithmetic that would break bit-exactness.
This PR is EDF/BDF only. An earlier revision of it also touched
_read_segments_file, BrainVision and EEGLAB at once, which was too much toreview in one go; the rest was split out and measured separately:
not depend on this PR.
_read_segments_filemmap + a threaded read are not proposed anywhere yet.No MNE fixture is large enough to reach the 64 MB threading threshold (the
largest
.eegis 3.7 MB), so they were untested at any realistic scale.mne.parallel.parallel_func(prefer="threads")already exists for exactly this,so it should reuse that and ship its own benchmark rather than add a second
ThreadPoolExecutorto the codebase.sizing already bounds temporaries under 50 MB of its 64 MB cap, even for an
adversarial 2-channel/6-hour file with reversed picks.
Why this replaces #14215
#14215 added an optional
engine="edfio"toread_raw_edfto get EDF preloadsfrom ~453 ms down to ~368 ms, at the cost of an optional dependency, a new public
parameter, and a restricted feature set (uniform rates only, all channels EEG, no
meas_date). The native decoder here is faster than that on the same fixturewhile keeping every EDF feature, so the engine switch is no longer worth its
maintenance surface. Closing #14215 in favour of this.
Additional information
AI disclosure: I directed the work and reviewed and tested everything; Claude Code
(Claude Opus 5) wrote most of the code edits and the benchmark harness under my
direction.