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

FIX/DOC: read_ch_adjacency and "picks" argument #12066

Merged
merged 27 commits into from
Oct 5, 2023
Merged

Conversation

zubara
Copy link
Contributor

@zubara zubara commented Oct 4, 2023

@welcome
Copy link

welcome bot commented Oct 4, 2023

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

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Looks like a good start! Can you add an entry to doc/changes/devel.rst using the :newcontrib: role?

Also any change should ideally have an accompanying test. In this case for example you could modify:

mne/channels/tests/test_channels.py:def test_read_ch_adjacency(tmp_path):

and make sure passing a list of channel names works

mne/channels/channels.py Outdated Show resolved Hide resolved
@zubara zubara requested a review from drammock as a code owner October 4, 2023 11:41
doc/changes/devel.rst Outdated Show resolved Hide resolved
mne/channels/channels.py Outdated Show resolved Hide resolved
mne/channels/tests/test_channels.py Outdated Show resolved Hide resolved
@zubara zubara requested a review from drammock October 5, 2023 14:16
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.

changes look good, let's see now if the CIs are happy

mne/channels/channels.py Outdated Show resolved Hide resolved
Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

Qt test failure is unrelated and fixed in #12076, thanks @zubara !

@larsoner larsoner merged commit 27d1c59 into mne-tools:main Oct 5, 2023
26 of 28 checks passed
@welcome
Copy link

welcome bot commented Oct 5, 2023

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

@drammock drammock mentioned this pull request Oct 5, 2023
larsoner added a commit to larsoner/mne-python that referenced this pull request Oct 10, 2023
* 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)
  ...
@sappelhoff sappelhoff changed the title #11608, buggfix and docstring update FIX/DOC: read_ch_adjacency and "picks" argument Nov 13, 2023
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <[email protected]>
Co-authored-by: Daniel McCloy <[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.

4 participants