Skip to content

DOC: clean up ECG ICA docstrs#12866

Merged
larsoner merged 2 commits intomne-tools:mainfrom
sappelhoff:docfix
Sep 23, 2024
Merged

DOC: clean up ECG ICA docstrs#12866
larsoner merged 2 commits intomne-tools:mainfrom
sappelhoff:docfix

Conversation

@sappelhoff
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff commented Sep 21, 2024 •
edited
Loading

btw: Does anyone know why the automatic ECG channel creation only works for MEG channels? It would be really nice/helpful to also support this for EEG channels.

With references like this one, it seems possible: https://doi.org/10.1016/j.cmpb.2019.105092

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Sep 21, 2024

The average of all magnetometers is a good proxy for an ECG channel. It does not well for EEG or planar gradiometers

sappelhoff
sappelhoff commented Sep 23, 2024
View reviewed changes
@sappelhoff
Copy link
Copy Markdown
Member Author

sappelhoff commented Sep 23, 2024

Test failures are unrelated

FAILED mne/decoding/tests/test_receptive_field.py::test_rf_sklearn_compliance[<ReceptiveField|tmin,tmax:(-1.000,2.000),estimator:<class'sklearn.linear_model._ridge.Ridge'>,fit:False>-check_estimators_unfitted] - AssertionError: Estimator should raise a NotFittedError when calling predict before fit. Either call check_is_fitted(self) at the beginning of predict or set tags.requires_fit=False on estimator tags to disable this check.

larsoner
larsoner approved these changes Sep 23, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Even though @agramfort said:

It does not [work] well for EEG or planar gradiometers

It looks like we do allow it for planar gradiometers, so I'll go ahead and merge since the changes more accurately reflect what we currently do. Maybe in some follow-up PR we could consider removing the grad support, or at least making it opt-in instead of default.

@larsoner larsoner merged commit ab25168 into mne-tools:main Sep 23, 2024
@larsoner
Copy link
Copy Markdown
Member

larsoner commented Sep 23, 2024

Thanks @sappelhoff !

@sappelhoff sappelhoff deleted the docfix branch September 23, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@larsoner larsoner larsoner approved these changes

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@cbrnr cbrnr Awaiting requested review from cbrnr

@adam2392 adam2392 Awaiting requested review from adam2392

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@dengemann dengemann Awaiting requested review from dengemann

+1 more reviewer

@hoechenberger hoechenberger hoechenberger approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

no-changelog-entry-needed skip-changelog-checks

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sappelhoff @agramfort @larsoner @hoechenberger