-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
… 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()
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
thanks for taking a stab at this @btkcodedev! |
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 |
Co-authored-by: Marijn van Vliet <[email protected]>
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. |
@btkcodedev Could you review the sentence modification I proposed and add a changelog entry?
|
Co-authored-by: Mathieu Scheltienne <[email protected]>
Update names.inc
Create 12326.other.rst
@mscheltienne Changes done :) |
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.
Thank you @btkcodedev
2 more minor comments and this will be good to merge!
Co-authored-by: Mathieu Scheltienne <[email protected]>
@wmvanvliet Could you please review again? Currently your change request (which AFAICT has been addressed) is blocking the merge. Thanks! |
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.
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`. |
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'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.
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 |
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.
FYI parentheses get added automatically when you use the :meth:
role
# Note that the method :meth:`~mne.io.Raw.pick()` default | |
# Note that the method :meth:`~mne.io.Raw.pick` default |
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.
Once @drammock's comments are resolved, LGTM
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
Didn't know there was an automerge in place. Perhaps we can fix @drammock's comments directly in main? |
Thanks for your contribution, @btkcodedev!! |
Somehow there is in
I'm going to delete the |
@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. |
…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]>
Reference issue
Closes #12158
Updated the tutorial by removing legacy method call of mne.io.Raw.pick_types() to instance.pick()