Repository navigation
ENH: Allow passing precomputed interpolation data for EEG and MEG (#4651) - #14455
Conversation
0d6b2c7 to
1242f1b
Compare
d485565 to
40c5a0e
Compare
|
I actually don't think this so far does any caching, so at least the title of the PR should change. This allows passing precomputed matrices but doesn't do any caching at the MNE side. For this:
This isn't really enough to ensure compat for EEG. You should really use the channel locations Looking ahead to #14464, I think it would make sense to make We should also explicitly state in the docstring that if I am not sure we need |
40c5a0e to
da2199b
Compare
|
thanks for the review @larsoner! i have updated the PR to address all your points:
all 46 tests in |
|
In the most extreme benchmark condition (256 channels), aa 35x speedup sounds great at first. But even so, we're talking about a 45ms absolute difference between In your benchmark, how many channels are marked bad and thus interpolated? Can you provide a MWE that we can copy-paste in a terminal or script to run ourselves? |
bff0679 to
c049639
Compare
|
Hi @scott-huberty, In the benchmark, about 5% of the channels were marked as bad (2 channels for 32ch, 4 for 64ch, 6 for 128ch and 12 for 256ch). For a single evoked object, or for interactive work in a notebook, the difference between 45 ms and a couple of ms is not noticeable. The motivation for #4651 and for this API is batch and automated processing:
Below is a small example you can run directly in a terminal. It uses the built-in import time
import mne
import numpy as np
from mne.channels import compute_bad_channel_interpolation_matrix
# Setup standard 128-channel EEG montage
montage = mne.channels.make_standard_montage("biosemi128")
ch_names = montage.ch_names
info = mne.create_info(ch_names=ch_names, sfreq=1000.0, ch_types="eeg")
raw = mne.io.RawArray(
np.random.RandomState(42).randn(len(ch_names), 1000), info, verbose=False
)
raw.set_montage(montage, verbose=False)
# Mark ~5% bad channels (6 bad channels)
raw.info["bads"] = ch_names[5:11]
n_iter = 50
# Case A: On-the-fly recomputation (main)
t0 = time.perf_counter()
for _ in range(n_iter):
raw.copy().interpolate_bads(verbose=False)
t_recompute = (time.perf_counter() - t0) / n_iter
# Case B: Precomputed matrix (this PR)
interp = compute_bad_channel_interpolation_matrix(raw, verbose=False)
t0 = time.perf_counter()
for _ in range(n_iter):
raw.copy().interpolate_bads(interpolation=interp, verbose=False)
t_precomputed = (time.perf_counter() - t0) / n_iter
print(f"128 ch (6 bads):")
print(f" - On-the-fly recompute: {t_recompute * 1000:6.2f} ms")
print(f" - Precomputed matrix: {t_precomputed * 1000:6.2f} ms")
print(f" - Speedup: {t_recompute / t_precomputed:6.1f}x") |
|
OK I am convinced ; ) |
| dict(eeg=interp), | ||
| dict(eeg=interp["eeg"]), | ||
| dict(eeg=interp["eeg"]["matrix"]), |
There was a problem hiding this comment.
I don't think we should allow all these. Let's just support dict with str key mapping to an InterpolationMatrix. It's simpler and allows checking correctness better.
There was a problem hiding this comment.
addressed in be24669. interpolation must now be a dict that maps the channel type strings ('eeg') to InterpolationMatrix instances (for example interpolation=dict(eeg=interp)). if you pass an unkeyed object or a raw ndarray, it is rejected with a TypeError that has a clear message, and i have updated the tests to match.
| **kwargs : dict | ||
| Channel types mapped to interpolation dicts or matrices. |
There was a problem hiding this comment.
**kwargs makes this too broad an ambiguous. Better to just have an explicit API with keys that have None values by default.
There was a problem hiding this comment.
updated in be24669. replaced **kwargs with explicit keyword arguments defaulting to None:
def __init__(
self,
*,
matrix=None,
bads=None,
goods_idx=None,
bads_idx=None,
ch_names=None,
pos=None,
origin=None,
method=None,
):the numpydoc parameter list has also been updated to document each argument explicitly.
| len(step.get("bads", [])) | ||
| for step in self.values() | ||
| if isinstance(step, dict) |
There was a problem hiding this comment.
addressed in be24669. because InterpolationMatrix now directly represents the interpolation state of a channel type rather than wrapping nested dictionaries, __repr__ accesses self.get("bads") directly:
def __repr__(self):
bads = self.get("bads")
if bads is None and self.get("matrix") is None:
return "<InterpolationMatrix | empty>"
n_bads = len(bads) if bads is not None else 0
plural = "s" if n_bads != 1 else ""
return f"<InterpolationMatrix | {n_bads} bad channel{plural}>"|
Okay thinking about this more, it computes an interpolation that depends on the bad channel set, which unfortunately does not serve the |
|
thanks @larsoner, that makes complete sense! precomputing a static bads-dependent matrix indeed doesn't help precomputing the bads-independent geometry object per channel type is mathematically much cleaner and avoids all the heavy legendre polynomial evaluations:
i tested both on sample data:
i'm proceeding with this implementation right away so you have a working pr to review:
i'll push the updated commits with tests once ready, and we can easily tweak any function or container naming if you have a specific preference! |
be24669 to
aea9028
Compare
|
i have updated the pr to implement this bads-independent geometry precomputation architecture (commit
|
cae14d3 to
56d2536
Compare
…ne-tools#4651) - Add compute_interpolation_data to precompute bads-independent geometry: - For EEG: full spherical spline G matrix (via _calc_g) across all EEG channels. - For MEG: full coil lead field dot products matrix (via _do_self_dots). - Allow passing interpolation=dict(eeg=..., meg=...) or InterpolationData instance to interpolate_bads on Raw, Epochs, and Evoked. - Dynamically slice G and coil dots to interpolate arbitrary bad channel sets on the fly, serving leave-one-out cross-validation and per-epoch bad channel workflows (e.g., autoreject) without recomputing Legendre series. - Add comprehensive test coverage and documentation.
56d2536 to
bbc0e63
Compare
|
@ishaaaan17 I took a stab at simplifying things, cutting +519 -37 down to +260 -21. Can you see if the changes make sense to you? |
|
@ishaaaan17 it also looks like your comment:
Was probably copy-pasted directly from an LLM prompt output. Please make sure you're replying with your own words! |
|
sorry about that @larsoner, my mistake actually, I had drafted my notes and used Claude only for formatting the markdown equations and tables. while copying, I carelessly pasted the trailing assistant note also and didn't notice it. I understand your concern. all the code, profiling and benchmarking numbers in the commit are my own work. thanks for pointing this out. regarding |
|
Okay let's move forward with this then, thanks @ishaaaan17 ! |
what does this implement/fix?
addresses #4651 (precompute bad channel interpolation data for eeg and meg).
when preprocessing multi-condition
Evokederp datasets, batches ofEpochs, or running automated artifact pipelines likeautoreject, calling.interpolate_bads()repeatedly on data with identical sensor configurations recomputes:_do_self_dots) for meg.this pr implements precomputation for both modalities:
mne.channels.compute_interpolation_data(info, *, origin="auto", mode="accurate"), returning anInterpolationData(subclass ofdict) containing precomputed spline/field matrices and sensor selection metadata.interpolation=Noneto.interpolate_bads()acrossRaw,Epochs, andEvoked.badsmatch, cleanly falling back to normal recomputation if sensors change.atol=1e-15for eeg,atol=1e-12for meg) compared to the uncached path.example usage
benchmark results
atol=1e-15)atol=1e-15)atol=1e-15)atol=1e-15)atol=1e-12)atol=1e-12)atol=1e-12)verification
pytest mne/channels/tests/test_interpolation.pypasses cleanly for both eeg and meg.ruff check,ruff format --check, andcodespellpass with no warnings.additional information
assisted by google antigravity / claude for drafting and benchmark script scaffolding; code and numerical parity were manually reviewed, verified, and profiled locally against the uncached baseline.