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.

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

Jump to bottom

Allow not dropping bads when creating or plotting Spectrum objs #12006

Merged
merged 20 commits into from
Oct 4, 2023
Merged

Allow not dropping bads when creating or plotting Spectrum objs #12006

merged 20 commits into from
Oct 4, 2023

Conversation

GonReina
Copy link
Contributor

@GonReina GonReina commented Sep 21, 2023 •
edited by larsoner
Loading

Closes #11722
Closes #11382

As mentioned in the issue by @drammock :

  1. I modified raw.compute_psd() to take in an exclude argument. The default behaviour is exclude=().
  2. Spectrum.pick() has an exclude=(). It cannot inherit this from the aw/Epochs/Evoked data source.
    However, channels in exclude are removed when raw.compute_psd() is called so does it need to inherit from the data source? In which case maybe set exclude to be a property of Spectrum so it can be accessed by pick(). Maybe there is an easier way.
  3. 'Spectrum.plot()' has an exclude argument as of version 1.5 so no need to add it

I tested this changes locally with some test data and raw.compute_psd() behaves as expected. Only removing bad channels
unless specified by exclude="bads". This being my first contribution, please do give me some feedback where needed, thanks for your time!

@welcome
Copy link

welcome bot commented Sep 21, 2023

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

@drammock drammock changed the title Closes 11722 Allow not dropping bads when creating or plotting Spectrum objs Sep 26, 2023
drammock
drammock reviewed Sep 26, 2023
View reviewed changes
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.

Thanks for this! It looks pretty good! See some inline comments below. In addition: the main thing missing is we need to add a test. Test should go in mne/time_frequency/tests/test_spectrum.py probably. Feel free to ask for as much guidance as you need on the test.

Also, please merge in current main as there have been a few upstream changes since you opened this PR.

Finally: it's generally not considered good practice to open PRs from your main branch (see second paragraph here: https://mne.tools/dev/install/contributing.html#basic-git-commands). Just a heads up for next time.

doc/changes/names.inc Outdated
@@ -575,3 +575,5 @@
.. _Zhi Zhang: https://github.com/tczhangzhi/

.. _Zvi Baratz: https://github.com/ZviBaratz

.. _Gonzalo Reina: https://greina.me/
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding yourself here! We try to keep this file alphabetized, can you move your name up to the right place?

Also: need a changelog entry in doc/changes/devel.rst

@larsoner larsoner added this to the 1.6 milestone Oct 1, 2023
@GonReina
Copy link
Contributor Author

GonReina commented Oct 2, 2023

Hello! Thanks a lot for the feedback. I moved my name on the list and wrote a test to check that bads are not dropped unless specified with exclude="bads". Ran the test and looks fine. Let me know what you think! As for the branching, my apologies. I will create a feature branch next time!

drammock
drammock reviewed Oct 2, 2023
View reviewed changes
mne/time_frequency/tests/test_spectrum.py Outdated Show resolved Hide resolved
mne/time_frequency/tests/test_spectrum.py Outdated Show resolved Hide resolved
GonReina
GonReina commented Oct 3, 2023
View reviewed changes
Copy link
Contributor Author

@GonReina GonReina left a comment

Choose a reason for hiding this comment

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

Thanks for the feedback. Yeah I wasn't sure which fields I was able to change so just wanted to make sure. Have updated the test now and ran it, seemed fine!

@drammock drammock enabled auto-merge (squash) October 3, 2023 23:31
@drammock
Copy link
Member

drammock commented Oct 3, 2023

I pushed a minor tweak to the test and merged in current upstream main. Marking for merge-when-CIs-pass, thanks @Gon-reina !

@GonReina
Copy link
Contributor Author

GonReina commented Oct 4, 2023

Thanks a lot, I haven't got much experience testing so apologies in advance. I've updated main again so hopefully that does it!

@drammock drammock disabled auto-merge October 4, 2023 12:38
@drammock drammock enabled auto-merge (squash) October 4, 2023 13:27
@drammock drammock merged commit d79606b into mne-tools:main Oct 4, 2023
28 checks passed
@welcome
Copy link

welcome bot commented Oct 4, 2023

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

larsoner added a commit to larsoner/mne-python that referenced this pull request Oct 10, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into kernel
f952d58 
* upstream/main: (37 commits)
  Use constrained layout in matplotlib visualization (mne-tools#12050)
  Add raw stc (mne-tools#12001)
  [MRG] update codeowners (mne-tools#12089)
  DOC: Morlet wavelet length in tfr_morlet (mne-tools#12073)
  BUG: Fix bug with mne browser backend (mne-tools#12078)
  Cache avatars (mne-tools#12077)
  BUG: Fix bug with ch_name resolution (mne-tools#12086)
  add unicode roundtrip for FIF (mne-tools#12080)
  add Ivan to names.inc (mne-tools#12081)
  MAINT: Work around PySide 6.5.3 event loop error (mne-tools#12076)
  mne-tools#11608, buggfix and docstring update (mne-tools#12066)
  MAINT: Fix broken examples (mne-tools#12074)
  Add UI Event linking to DraggableColorbar (mne-tools#12057)
  handle lazy loading through .pyi type stubs (mne-tools#12072)
  BUG: Fix bug with sensor_colors (mne-tools#12068)
  clean  up some deprecations (mne-tools#12067)
  Allow not dropping bads when creating or plotting Spectrum objs (mne-tools#12006)
  Collapsible html repr for raw/info (mne-tools#12064)
  BUG: Fix bug with pickling MNEBadsList (mne-tools#12063)
  add details for Denis (mne-tools#12065)
  ...
@drammock drammock mentioned this pull request Oct 25, 2023
Closed
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
@GonReina @pre-commit-ci
@drammock @snwnde
Allow not dropping bads when creating or plotting Spectrum objs (mne-…
0515b02 
…tools#12006)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel McCloy <dan@mccloy.info>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drammock drammock drammock left review comments

@larsoner larsoner Awaiting requested review from larsoner larsoner is a code owner

@agramfort agramfort Awaiting requested review from agramfort agramfort is a code owner

@cbrnr cbrnr Awaiting requested review from cbrnr cbrnr is a code owner

@adam2392 adam2392 Awaiting requested review from adam2392 adam2392 is a code owner

@mscheltienne mscheltienne Awaiting requested review from mscheltienne mscheltienne is a code owner

@dengemann dengemann Awaiting requested review from dengemann dengemann is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.6
3 participants
@GonReina @drammock @larsoner