Skip to content
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

chore(docs): update preprocessing tutorial for using inst.pick() instead of pick_types() #12326

Merged
merged 18 commits into from
Jan 25, 2024

Conversation

btkcodedev
Copy link
Contributor

Reference issue

Closes #12158

Updated the tutorial by removing legacy method call of mne.io.Raw.pick_types() to instance.pick()

… pick_types()

Updated the tutorial by removing legacy method call of mne.io.Raw.pick_types() to instance.pick()
chore(docs): update preprocessing tutorial for using inst.pick() instead of pick_types()
Copy link

welcome bot commented Dec 26, 2023

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

@wmvanvliet
Copy link
Contributor

thanks for taking a stab at this @btkcodedev!

@hoechenberger
Copy link
Member

Thanks for your contribution! I think the beginning of the sentence must also be adjusted, since we don't pass an "exclude" parameter anywhere as far as I can see

@hoechenberger
Copy link
Member

Thanks, but I don't think the sentence makes too much sense the way it's phrased now. I think it would be better to just write that "pick()" retains the bad channels. It's just how the methods works, not the justification why we used it.

@mscheltienne
Copy link
Member

@btkcodedev Could you review the sentence modification I proposed and add a changelog entry?
For the chanbgelog entry, you can refer to this section of the contributing guide. You need to add yourself to the author list (alphabetical order) and to add a file in this folder, named 12326.other.rst, describe the change in the file and add yourself as author, e.g.

Short description of the changes, by :newcontrib:Firstname Lastname.

@btkcodedev
Copy link
Contributor Author

@mscheltienne Changes done :)

Copy link
Member

@mscheltienne mscheltienne left a comment

Choose a reason for hiding this comment

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

Thank you @btkcodedev
2 more minor comments and this will be good to merge!

doc/changes/devel/12326.other.rst Outdated Show resolved Hide resolved
doc/changes/names.inc Outdated Show resolved Hide resolved
@mscheltienne mscheltienne enabled auto-merge (squash) January 22, 2024 11:07
@hoechenberger
Copy link
Member

@wmvanvliet Could you please review again? Currently your change request (which AFAICT has been addressed) is blocking the merge. Thanks!

Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

just two small nitpicks from me. Thanks for fixing this!

@@ -0,0 +1 @@
Updated the text in the preprocessing tutorial to use :class:`mne.io.Raw.pick()` instead of the legacy :class:`mne.io.Raw.pick_types()`, by :newcontrib:`btkcodedev`.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little surprised that these cross-refs worked; regardless, it's more accurate to mark these with the :meth: role since they're links to methods (not classes). The parentheses will get added automatically.

Suggested change
Updated the text in the preprocessing tutorial to use :class:`mne.io.Raw.pick()` instead of the legacy :class:`mne.io.Raw.pick_types()`, by :newcontrib:`btkcodedev`.
Updated the preprocessing tutorial to use :meth:`mne.io.Raw.pick` instead of the legacy :meth:`mne.io.Raw.pick_types`, by :newcontrib:`btkcodedev`.

@@ -238,8 +238,9 @@
fig.suptitle(title, size="xx-large", weight="bold")

# %%
# Note that we used the ``exclude=[]`` trick in the call to
# :meth:`~mne.io.Raw.pick_types` to make sure the bad channels were not
# Note that the method :meth:`~mne.io.Raw.pick()` default
Copy link
Member

Choose a reason for hiding this comment

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

FYI parentheses get added automatically when you use the :meth: role

Suggested change
# Note that the method :meth:`~mne.io.Raw.pick()` default
# Note that the method :meth:`~mne.io.Raw.pick` default

Copy link
Contributor

@wmvanvliet wmvanvliet left a comment

Choose a reason for hiding this comment

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

Once @drammock's comments are resolved, LGTM

@mscheltienne mscheltienne merged commit f060e6b into mne-tools:main Jan 25, 2024
29 checks passed
Copy link

welcome bot commented Jan 25, 2024

🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪

@wmvanvliet
Copy link
Contributor

Didn't know there was an automerge in place. Perhaps we can fix @drammock's comments directly in main?

@hoechenberger
Copy link
Member

Thanks for your contribution, @btkcodedev!!

@larsoner
Copy link
Member

Somehow there is in mne-tools/mne-python :

$ git fetch upstream
...
 From github.com:mne-tools/mne-python
   03d78f43bf..990ce18d4e  main              -> upstream/main
 * [new branch]            revert-12326-main -> upstream/revert-12326-main

I'm going to delete the revert-12326-main assuming perhaps @wmvanvliet or someone else considered reverting for a minute there. Agree with @wmvanvliet that final comments can be addressed quickly elsewhere, but maybe just as part of another PR when someone opens one (rather than a push to main which is always a bit dicey)

@mscheltienne
Copy link
Member

@wmvanvliet @larsoner I think I addressed the final comments before going on holidays in a PR, not yet merged, which was blocked by the towncrier job failing.

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
…ead of pick_types() (mne-tools#12326)

Co-authored-by: Marijn van Vliet <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
Co-authored-by: Mathieu Scheltienne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Small error in interpolation tutorial
6 participants