Skip to content

ENH: replace _get_blocks binary reader with mffpy Reader API#14043

Merged
scott-huberty merged 9 commits into
mne-tools:mainfrom
PragnyaKhandelwal:egi-mff-get-blocks-cleanup
Jul 13, 2026
Merged

ENH: replace _get_blocks binary reader with mffpy Reader API#14043
scott-huberty merged 9 commits into
mne-tools:mainfrom
PragnyaKhandelwal:egi-mff-get-blocks-cleanup

Conversation

@PragnyaKhandelwal

@PragnyaKhandelwal PragnyaKhandelwal commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Reference issue (if any)

Phase 2 of #13926

What does this implement/fix?

_get_blocks in general.py manually parsed the EGI binary signal
file block-by-block to extract sample counts, channel count, and
sampling rate. This duplicates functionality that mffpy already provides
through its Reader API.

This PR replaces both the EEG and PNS calls to _get_blocks in
_read_mff_header with mffpy.Reader, which is already a dependency:

  • reader.num_channels["EEG"] → replaces manual n_channels parsing
  • reader.sampling_rates["EEG"] → replaces manual sfreq parsing
  • reader.block_sample_counts["EEG"] → replaces samples_block for EEG
  • reader.block_sample_counts["PNSData"] → replaces samples_block for PNS

The same Reader instance (created once from the MFF directory) covers
both EEG and PNS, so no extra I/O is added. _get_blocks is no longer
imported in egimff.py as a result.

The imports inside _read_mff_header follow the existing lazy-import
pattern: _soft_import as a pre-check, then specific
from mffpy import Reader and from mffpy.xml_files import XML.

Additional information

@PragnyaKhandelwal
PragnyaKhandelwal marked this pull request as ready for review July 10, 2026 14:49
@PragnyaKhandelwal PragnyaKhandelwal mentioned this pull request Jul 10, 2026
Open
18 tasks
@PragnyaKhandelwal

PragnyaKhandelwal commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Test failure seems unrelated @scott-huberty....

image

@PragnyaKhandelwal PragnyaKhandelwal changed the title ENH: replace _get_blocks binary reader with mffpy Reader API ENH: replace _get_blocks binary reader with mffpy Reader API Jul 11, 2026
@scott-huberty

scott-huberty commented Jul 11, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

test failure seems unrelated @scott-huberty

Yep, see scikit-learn/scikit-learn#34458 . One of our CI's tests against the latest builds of Numpy, spicy, scikit etc. (so that we can catch bugs early), and a recent build of scikit-learn has not been published to the archive in a month or so.

@PragnyaKhandelwal

PragnyaKhandelwal commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

test failure seems unrelated @scott-huberty

Yep, see scikit-learn/scikit-learn#34458 . One of our CI's tests against the latest builds of Numpy, spicy, scikit etc. (so that we can catch bugs early), and a recent build of scikit-learn has not been published to the archive in a month or so.

Thanks for the information ))!!

@scott-huberty
scott-huberty self-requested a review July 11, 2026 18:21
@PragnyaKhandelwal

PragnyaKhandelwal commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

All checks are passing now @scott-huberty!

scott-huberty
scott-huberty approved these changes Jul 13, 2026
View reviewed changes

@scott-huberty scott-huberty left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's awesome seeing all the LOC we get to delete when offloading the work to mffpy!

@scott-huberty
scott-huberty merged commit 3b722aa into mne-tools:main Jul 13, 2026
41 of 42 checks passed
@PragnyaKhandelwal

PragnyaKhandelwal commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

It's awesome seeing all the LOC we get to delete when offloading the work to mffpy!

yep completely agree...general.py is reduced to more than half of what it was before....

larsoner added a commit to larsoner/mne-python that referenced this pull request Jul 14, 2026
@larsoner
Merge remote-tracking branch 'upstream/main' into types
6a5387c 
* upstream/main:
  Make scrollbar handlers draggable (mne-tools#14040)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#14052)
  Warn when Epochs events fall outside the raw data range (mne-tools#12989) (mne-tools#14004)
  Ensure epochs being concatenated have compatible event ids (mne-tools#14051)
  Widen main content area (mne-tools#14015)
  MAINT: remove dead gain/bits/value_range fields from _read_header in … (mne-tools#14047)
  ENH: replace `_get_blocks` binary reader with mffpy Reader API (mne-tools#14043)
  MAINT: Update dependency specifiers (mne-tools#14048)
  [dependabot]: Bump the actions group with 2 updates (mne-tools#14049)
  Simplify doc building with more refleak (mne-tools#14045)
  ENH: add overlay Brain GUI (mne-tools#14031)
  ENH: support multiple simultaneous overlays in Brain.add_data (mne-tools#13995)
  Add option to show a zero line in browser (mne-tools#14018)
  FIX: pass cmap name string not tuple to interactive topomap slider kwargs (mne-tools#14039)
  Doc/add ai policy pointer (mne-tools#14037)
  Allow subclasses of FigureClass to be passed to plot_raw/plot_epochs (mne-tools#13979)
  MAINT: Replace manual PNS binary block reader in `_read_segment_file` with `mffpy` (mne-tools#14030)
larsoner added a commit to sharifhsn/mne-python that referenced this pull request Jul 14, 2026
@larsoner
Merge remote-tracking branch 'upstream/main' into sharifhsn-perf-opt
b07384e 
* upstream/main: (206 commits)
  Improve type checks (mne-tools#14036)
  Make scrollbar handlers draggable (mne-tools#14040)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#14052)
  Warn when Epochs events fall outside the raw data range (mne-tools#12989) (mne-tools#14004)
  Ensure epochs being concatenated have compatible event ids (mne-tools#14051)
  Widen main content area (mne-tools#14015)
  MAINT: remove dead gain/bits/value_range fields from _read_header in … (mne-tools#14047)
  ENH: replace `_get_blocks` binary reader with mffpy Reader API (mne-tools#14043)
  MAINT: Update dependency specifiers (mne-tools#14048)
  [dependabot]: Bump the actions group with 2 updates (mne-tools#14049)
  Simplify doc building with more refleak (mne-tools#14045)
  ENH: add overlay Brain GUI (mne-tools#14031)
  ENH: support multiple simultaneous overlays in Brain.add_data (mne-tools#13995)
  Add option to show a zero line in browser (mne-tools#14018)
  FIX: pass cmap name string not tuple to interactive topomap slider kwargs (mne-tools#14039)
  Doc/add ai policy pointer (mne-tools#14037)
  Allow subclasses of FigureClass to be passed to plot_raw/plot_epochs (mne-tools#13979)
  MAINT: Replace manual PNS binary block reader in `_read_segment_file` with `mffpy` (mne-tools#14030)
  Fix transition bandwidth reported in 'filter too short' error (mne-tools#11406) (mne-tools#14005)
  ENH: Show the current time as a vertical line in plot_evoked_topo (mne-tools#14032)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scott-huberty scott-huberty scott-huberty approved these changes
@larsoner larsoner Awaiting requested review from larsoner larsoner is a code owner
@drammock drammock Awaiting requested review from drammock drammock is a code owner
@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@PragnyaKhandelwal @scott-huberty