Skip to content

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.

Sign up for GitHub

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

Merged
sappelhoff merged 3 commits into from
Jun 2, 2025
Merged

FIX, DOC: Drop bad channel in 10_publication_figure.py #13266

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
353bc8a
Drop bad channel in 10_publication_figure.py
jona-sassenhagen May 31, 2025
807daec
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] May 31, 2025
5b091b8
Switched from explicit drop_channels to using exclude in pick
jona-sassenhagen Jun 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tutorials/visualization/10_publication_figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
fname_evoked = data_path / "MEG" / "sample" / "sample_audvis-ave.fif"

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))
Copy link
Copy Markdown
Member

@sappelhoff sappelhoff Jun 2, 2025

Choose a reason for hiding this comment

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

👋 @jona-sassenhagen

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)

Copy link
Copy Markdown
Contributor Author

@jona-sassenhagen jona-sassenhagen Jun 2, 2025

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:

Screenshot 2025-06-02 at 13 07 49

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.

Copy link
Copy Markdown
Member

@sappelhoff sappelhoff Jun 2, 2025

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.

Copy link
Copy Markdown
Contributor Author

@jona-sassenhagen jona-sassenhagen Jun 2, 2025

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 ...

Copy link
Copy Markdown
Member

@sappelhoff sappelhoff Jun 2, 2025

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.

Copy link
Copy Markdown
Contributor Author

@jona-sassenhagen jona-sassenhagen Jun 2, 2025

Choose a reason for hiding this comment

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

👍

Merge?

Copy link
Copy Markdown
Contributor Author

@jona-sassenhagen jona-sassenhagen Jun 2, 2025

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.

Copy link
Copy Markdown
Member

@sappelhoff sappelhoff Jun 2, 2025

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.

Copy link
Copy Markdown
Contributor Author

@jona-sassenhagen jona-sassenhagen Jun 2, 2025

Choose a reason for hiding this comment

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

🙏

Copy link
Copy Markdown
Member

@sappelhoff sappelhoff Jun 2, 2025 •
edited
Loading

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.

mne-python/mne/utils/docs.py

Line 3441 in fc9078a

docdict["picks_all"] = _reflow_param_docstring(f"{picks_base} all channels. {reminder}")

max_t = evoked.get_peak()[1]

stc = mne.read_source_estimate(fname_stc)
Expand Down