-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
FIX, DOC: Drop bad channel in 10_publication_figure.py #13266
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
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
agramfort
left a comment
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.
hello 👋
|
|
||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, but I'm going to be honest, I'm not gonna fix the docs myself ...
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.
Sure, no problem. But if you (and others) agree, I could push that fix.
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.
👍
Merge?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
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.
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
| docdict["picks_all"] = _reflow_param_docstring(f"{picks_base} all channels. {reminder}") |
* 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, 👋