FIX, DOC: Drop bad channel in 10_publication_figure.py#13266
FIX, DOC: Drop bad channel in 10_publication_figure.py#13266sappelhoff merged 3 commits intomne-tools:mainfrom
Conversation
Visualisation tutorial 10 is broken because finding peaks without dropping bars finds a random point on a broken grad. This drops the bad channel before finding the peak.
for more information, see https://pre-commit.ci
|
|
||
| evoked = mne.read_evokeds(fname_evoked, "Left Auditory") | ||
| evoked.pick(picks="grad").apply_baseline((None, 0.0)) | ||
| evoked.pick(picks="grad").drop_channels(evoked.info["bads"]).apply_baseline((None, 0.0)) |
There was a problem hiding this comment.
I am surprised that this is needed 🤔 When reading the API docs:
https://mne.tools/stable/generated/mne.Evoked.html#mne.Evoked.pick
it seems to me that bad channels should be automatically dropped via .pick UNLESS explicitly selected via index or name (which is not what is happening here)
There was a problem hiding this comment.
Well, that's certainly not what's happening:
I also don't think that's implied by the docs:
exclude=()
excludelist | str
Set of channels to exclude, only used when picking based on types (e.g., exclude=”bads” when picks=”meg”).
To me this just says, if you pick based on types, then exclude will be used and can be set to bads.
Note () isn't a list.
There was a problem hiding this comment.
Yes, good point -- I was guided by:
Note that channels in info['bads'] will be included if their names or indices are explicitly provided.
I think this is something that must be improved in the docstr 🤔
-Note that channels in info['bads'] will be included if their names or indices are explicitly provided.
+Note that channels in info['bads'] will be included if their names or indices are explicitly provided,
+or when `picks` refers to a channel type. In the latter case,
+please use the `exclude` parameter to explicitly exclude specific channels.Maybe like this ☝️
Also, as you say the exclude parameter should be updated to also accept sets of channel names.
And I also find the formulation worthy of clarification:
-Set of channels to exclude, only used when picking based on types (e.g., exclude=”bads” when picks=”meg”).
+Only used when picking based on types. Which channels to exclude.
+Can be set to "bads" to exclude all channels marked as bad.There was a problem hiding this comment.
Right, but I'm going to be honest, I'm not gonna fix the docs myself ...
There was a problem hiding this comment.
Sure, no problem. But if you (and others) agree, I could push that fix.
There was a problem hiding this comment.
Ah to clarify
if you (and others) agree, I could push that fix.
I personally would make a PR of its own, with some more doc fixes, but up to you, I'd be ok with you hijacking this one.
There was a problem hiding this comment.
marked for auto-merge if all tests pass 👍 thanks in advance!
Yes, I will make a follow up PR.
There was a problem hiding this comment.
on the other hand, I just checked the docstrings on this topic and it is an absolute nightmare to edit, so unfortunately I do not have the time for that just now.
Line 3441 in fc9078a
* upstream/main: (55 commits) doc: fix rendering typo rst docstr (mne-tools#13301) DOC: fix docstrs around layout functions (mne-tools#13300) MAINT: Fix doc build failure due to deprecation (mne-tools#13299) Birthday input cast to datetime.date (mne-tools#13284) DOC: fix missing space, use f-strings, structure->object (mne-tools#13291) [pre-commit.ci] pre-commit autoupdate (mne-tools#13290) ENH: channel_indices_by_type now has an exclude param (mne-tools#13293) Proj id and proj name access (mne-tools#13261) Fix: nearly invisible traces with spatial_colors=True (mne-tools#13286) [pre-commit.ci] pre-commit autoupdate (mne-tools#13283) Bump autofix-ci/action from 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef to 635ffb0c9798bd160680f18fd73371e355b85f27 in the actions group (mne-tools#13282) fix Maxwell bads filtering (mne-tools#13280) fix actionable linkcheck errors (mne-tools#13273) MAINT: Use radius keyword with PyVista tube (mne-tools#13277) BUG: Fix bug with simulating head pos and BEM (mne-tools#13276) [pre-commit.ci] pre-commit autoupdate (mne-tools#13274) MAINT: Update code credit (mne-tools#13267) Annotations extras (mne-tools#13228) Tidy up the directory reading (mne-tools#13268) FIX, DOC: Drop bad channel in 10_publication_figure.py (mne-tools#13266) ...
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
If you look at tutorial 10, getting figures publication ready, the source space plots are empty. The clue to what's going on is in the vertical line to indicate the time of peak activation: it's in some seemingly random pre-baseline time window.
The reason is that evoked.get_peak is invoked without excluding the bad channel, which has extreme amplitude. I couldn't find the version where this problem was introduced. But dropping the bad channel gives a sensible peak instead (0.08 s instead of -0.03).
I can't actually fully build this example; something is broken for me where the vertical line is never updated. However, the source spaces now show actual activation.
Looking at this, it seems like it should be documented that
evoked.drop_channelsworks in place ...Also, 👋