-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH: Overlap-add processing for maxwell filter #13080
base: main
Are you sure you want to change the base?
Conversation
* upstream/main: (152 commits) FIX: missing channels/fiducials can be np.nan (mne-tools#11634) use py3.10 in precommit config (mne-tools#11648) MAINT: Unify GH Actions pytest (mne-tools#11644) MRG: Rename "Discourse" link in top navigation to "Forum" [ci skip] (mne-tools#11649) ENH: Add support for Harmonic Field correction (mne-tools#11536) Add pre-commit (mne-tools#11541) BUG: Fix bug with paths (mne-tools#11639) MAINT: Report download time and size (mne-tools#11635) MRG: Allow retrieval of channel names via make_1020_channel_selections() (mne-tools#11632) Fix index name in to_data_frame()'s docstring (mne-tools#11457) MAINT: Use VTK prerelease wheels in pre jobs (mne-tools#11629) ENH: Allow gradient compensated data in maxwell_filter (mne-tools#10554) make test compatible with future pandas (mne-tools#11625) Display SVG figures correctly in Report (mne-tools#11623) API: Port ieeg gui over to mne-gui-addons and add tfr gui example (mne-tools#11616) MAINT: Add token [ci skip] (mne-tools#11622) API: One cycle of backward compat (mne-tools#11621) MAINT: Use git rather than zipball (mne-tools#11620) ENH: Speed up code a bit (mne-tools#11614) [BUG, MRG] Don't modify info in place for transform points (mne-tools#11612) ...
* upstream/main: (32 commits) MAINT: Update download buttons [skip azp] [skip actions] [skip cirrus] Fix canvas.draw() in callback (mne-tools#11697) Remove recursion in plot_ica_components and use context manager for plt.ion/plt.ioff (mne-tools#11696) Update affiliation (mne-tools#11695) BUG: Fix bug with fwd restriction (mne-tools#11694) MRG: Suggest using "conda rename" in MNE updating instructions (mne-tools#11692) FIX: Regex [ci skip] MAINT: Apply deprecations [circle deploy] (mne-tools#11687) MAINT: Release 1.4.0 (mne-tools#11686) Trap music (mne-tools#11679) Fix call to plot_tfr_topomap from interactive AverageTFR.plot_topo function (mne-tools#11683) silence spectrum plot warning in examples/tutorials [circle full] (mne-tools#11682) Spectrum plot picks (mne-tools#11680) Update website conf (mne-tools#11675) BUG: Fix bug with MF LCMV rank (mne-tools#11664) ENH: Change known_config_types to dict (mne-tools#11166) MAINT: Improve README (mne-tools#11673) MAINT: Add to git-blame-ignore-revs [circle front] MAINT: Run black on codebase MAINT: Use black ...
* upstream/main: (24 commits) Allow int-like as ID of make_fixed_length_events (mne-tools#11748) Easycap-M43 montage (mne-tools#11744) ENH: Create a Calibrations class for eyetracking data (mne-tools#11719) Fix alphabetical order in overview/people.rst, fix sphinx formatting in docstrings and set verbose to keyword-only (mne-tools#11745) Add Mathieu Scheltienne to MNE-Python Steering Council (mne-tools#11741) removed requirement for curv.*h files to create Brain object (mne-tools#11704) [BUG] Fix mne.viz.Brain.add_volume_labels matrix ordering bug (mne-tools#11730) Fix installer links (mne-tools#11729) MAINT: Update for PyVista deprecation (mne-tools#11727) MAINT: Update roadmap (mne-tools#11724) MAINT: Update download link [skip azp] [skip cirrus] [skip actions] fix case for chpi_info[1] == None (mne-tools#11714) Add cmap argument for mne.viz.utils.plot_sensors (mne-tools#11720) BUG: Fix one more PySide6 bug (mne-tools#11723) MAINT: Fix PySide6 and PyVista compat (mne-tools#11721) MRG: If _check_fname() cannot find a file, display the path in quotation marks to help spot accidental trailing spaces (mne-tools#11718) Add "array-like" to `_validate_type()` (mne-tools#11713) MAINT: Avoid problematic PySide6 (mne-tools#11715) Fix installer links (mne-tools#11709) Updating change log after PR mne-tools#11575 (mne-tools#11707) ...
* origin/cola5: FIX: Uncomment
* upstream/main: (26 commits) MAINT: Better fix for deprecation (mne-tools#11789) udpate doc for 11786 (mne-tools#11787) MAINT: Ignore warning in example (mne-tools#11781) Keeping event names when combining channel on epochs objects (mne-tools#11786) DOC: Fix incorrect docs for annotate_movement [ci skip] (mne-tools#11779) Refresh eegbci code and docstrings (mne-tools#11783) DOC: EEG eye-tracking alignment tutorial (mne-tools#11770) [MRG] By-pass set_annotations when plotting ICA sources (mne-tools#11766) add missing changelog from 11773 (mne-tools#11777) Fix eeglab.py, an error for read annotations in `RawEEGLab` (mne-tools#11773) [MRG] Use FIFF.FIFF_UNITM_NONE instead of 0 when adding a reference channel (mne-tools#11774) MAINT: Update for linkcheck [circle linkcheck] (mne-tools#11771) ENH: Interpolate blinks in eyetrack channels (mne-tools#11740) MAINT: Work around joblib bug (mne-tools#11765) Update manual install docs (mne-tools#11764) add logging message about which EOG channel used for blink detection (mne-tools#11757) DOC: Fix docs (mne-tools#11756) BUG: Fix bug with SNR calculation (mne-tools#11755) BUG: Fix bug with cHPI off (mne-tools#11754) Change color from annotation to red if name changes to "bad_" or "edge_" (mne-tools#11753) ...
* upstream/main: (736 commits) ENH: Add round-trip channel name saving (mne-tools#13003) update governance (mne-tools#12896) pin selenium and stop filtering its warning (mne-tools#13000) DOC: fix return value doc of inst.get_montage() (mne-tools#12995) remove some ._filenames uses in favor of .filenames (mne-tools#12996) use eegbci.standardize instead of custom code (mne-tools#12997) Bump autofix-ci/action from dd55f44df8f7cdb7a6bf74c78677eb8acd40cd0a to ff86a557419858bb967097bfc916833f5647fa8c in the actions group (mne-tools#12999) MAINT: Update code credit (mne-tools#12998) MAINT: Unpin VTK (mne-tools#12991) [pre-commit.ci] pre-commit autoupdate (mne-tools#12988) Fix: update cnt.py to check missing annotation (mne-tools#12986) do not log about using set_montage AGAIN if it has never been used (mne-tools#12984) DOC: fix numpy docstr example Vectorizer() (mne-tools#12983) Changed epochs_clean_sub = model_plain.apply(epochs) to epochs_clean_sub = model_sub.apply(epochs), making use of the regression coefficients from model_sub. mne-tools#12977 (mne-tools#12978) DOC: add meegkit to related software suite (mne-tools#12976) DOC: specify tmax_raw param may be None, and what it means (mne-tools#12972) [pre-commit.ci] pre-commit autoupdate (mne-tools#12975) DOC: fix note in legacy pick_channels func (mne-tools#12973) Use `uint16_codec` argument (mne-tools#12971) Bump codecov/codecov-action from 4 to 5 in the actions group (mne-tools#12970) ...
* upstream/main: (57 commits) Allow lasso selection sensors in a plot_evoked_topo (mne-tools#12071) MAINT: Fix doc build (mne-tools#13076) BUG: Improve sklearn compliance (mne-tools#13065) [pre-commit.ci] pre-commit autoupdate (mne-tools#13073) MAINT: Add Numba to 3.13 test (mne-tools#13075) Bump autofix-ci/action from ff86a557419858bb967097bfc916833f5647fa8c to 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef in the actions group (mne-tools#13071) [BUG] Correct annotation onset for exportation to EDF and EEGLAB (mne-tools#12656) New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037) [BUG] Fix taper weighting in computation of TFR multitaper power (mne-tools#13067) [FIX] Reading an EDF with preload=False and mixed frequency (mne-tools#13069) Fix evoked topomap colorbars, closes mne-tools#13050 (mne-tools#13063) [pre-commit.ci] pre-commit autoupdate (mne-tools#13060) BUG: Fix bug with interval calculation (mne-tools#13062) [DOC] extend documentation for add_channels (mne-tools#13051) Add `combine_tfr` to API (mne-tools#13054) Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058) BUG: Fix bug with helium anon (mne-tools#13056) [ENH] Add option to store and return TFR taper weights (mne-tools#12910) BUG: viz plot window's 'title' argument showed no effect. (mne-tools#12828) MAINT: Ensure limited set of tests are skipped (mne-tools#13053) ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look through the code, but not with a fine-toothed comb. Nothing jumps out as questionable, but I will try to do a more thorough look (and testing) next week. I'm a bit undecided about changing the defaults, but I haven't actually looked at the results / SNR of this yet, which could easily sway me.
from mne.preprocessing import ( | ||
maxwell_filter as _maxwell_filter_ola, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] this doesn't need to be a separate import statement I think? locally I can do
from numpy import (
ndarray,
sum as npsum, # <---
rollaxis
)
and it works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, but ruff
wants it this way, it undoes the proposed change:
larsoner:~/python/mne-python$ git diff
diff --git a/mne/preprocessing/tests/test_maxwell.py b/mne/preprocessing/tests/test_maxwell.py
index 2f279440d1..ce28a04b0c 100644
--- a/mne/preprocessing/tests/test_maxwell.py
+++ b/mne/preprocessing/tests/test_maxwell.py
@@ -34,10 +34,8 @@ from mne.preprocessing import (
annotate_movement,
compute_maxwell_basis,
find_bad_channels_maxwell,
- maxwell_filter_prepare_emptyroom,
-)
-from mne.preprocessing import (
maxwell_filter as _maxwell_filter_ola,
+ maxwell_filter_prepare_emptyroom,
)
from mne.preprocessing.maxwell import (
_bases_complex_to_real,
larsoner:~/python/mne-python$ pre-commit run --all
ruff lint mne........................................................................Failed
- hook id: ruff
- files were modified by this hook
Found 1 error (1 fixed, 0 remaining).
...
larsoner:~/python/mne-python$ git diff
larsoner:~/python/mne-python$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
weird, ruff
# For backward compat and to be most like MaxFilter, we make "maxwell_filter" | ||
# the one that behaves like MaxFilter. _maxwell_filter is left to | ||
# be the advanced/better one. | ||
maxwell_filter = partial(_maxwell_filter_ola, st_overlap=False, mc_interp="zero") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so IIUC this would change after a deprecation cycle right? And also the smoothing of headpos also gets enabled by default with 1.11 release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah both get enabled by default in 1.11. I would be inclined to leave this after 1.11 actually to keep diff
and blame
clean, but we could do a bunch of renames to like maxwell_filter_old
or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you're leaning toward not changing the defaults in 1.11? For me it depends on how big is the gain + how universal is the gain for different datasets. If it's (almost) always a (fairly big) improvement, there's a clear case to change the defaults, and our job becomes making sure it's widely communicated. If it's only sometimes an improvement, then I'd vote to leave the defaults as-is. I'm not in favor of maxwell_filter_old
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do want to change the defaults to the new smooth ones. Gains should be near universal.
I'm just saying I don't plan on changing this test file in particular because it'll lead to a bunch of code churn and blame
loss. Like maxwell_filter(...)
in this file would continue to mean "non-smooth" and maxwell_filter_ola
would mean "smooth".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh, OK, sorry I didn't realize this was in a test file. everything makes sense now 🤦🏻
Co-authored-by: Daniel McCloy <[email protected]>
Algorithms that process data in sequential temporal windows benefit from overlap-add processing to reduce edge artifacts. OTP in our codebase already uses this. This PR adds that to
maxwell_filter
for tSSS and for movement compensation (which is trickier because the windows / estimation points themselves have variable duration / timing).The MF code changes will be hard to review 😞 Fortunately, the
maxwell_filter
tests we have are pretty extensive, so we can sleep well at night knowing they still pass with very few modifications. And we see some improvements in tests with what will be the new defaults in 1.11. In some ways I think the code actually gets a bit clearer by the use of classes, for example for movement compensation / head position a class makes sense because the head position is a state you want to keep track of and get information / data / methods for. That way not all aspects are controlled by a monolithic, multi-indented function like now -- there is still a multi-indented monster, but its content is much more limited and thus easier to follow.@drammock one point for debate is whether we should actually make the new defaults "smooth" (via deprecation) as currently implemented here. In theory and in my testing they are indeed better, but they will make us more different from standard maxwell filtering. But I think the gain in data SNR is probably worth it.