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

[MRG] Forward argument axes from plot_sensors to DigMontage.plot #11470

Merged
merged 12 commits into from
Feb 15, 2023
Merged

[MRG] Forward argument axes from plot_sensors to DigMontage.plot #11470

merged 12 commits into from
Feb 15, 2023

Conversation

mscheltienne
Copy link
Member

@mscheltienne mscheltienne commented Feb 13, 2023 •
edited
Loading

This PR enables:

montage = ...  # DigMontage
f, ax = plt.subplots(1, 1)
montage.plot(axes=ax)

I noticed it was missing when I tried to plot 3 montages on the same figure. One more step towards a unified plotting API 😃

Closes #11404

@mscheltienne
Copy link
Member Author

Rendered documentation:

@mscheltienne mscheltienne changed the title Forward argument axes from plot_sensors to DigMontage.plot [MRG] Forward argument axes from plot_sensors to DigMontage.plot Feb 13, 2023
larsoner
larsoner reviewed Feb 13, 2023
View reviewed changes
mne/viz/utils.py Outdated Show resolved Hide resolved
mne/viz/montage.py Outdated Show resolved Hide resolved
mne/channels/montage.py Outdated Show resolved Hide resolved
mne/channels/tests/test_montage.py Show resolved Hide resolved
@mscheltienne
Copy link
Member Author

@larsoner Agree for the comments, it's just that we need to be careful since Axes3D is an instance of Axes. But should I address those now.. I was a bit hasty on this, I now notice an on-going PR about this issue: #11404
We can wait for #11404 to be merged first, and I can add the additional changes on the updated main.

@larsoner
Copy link
Member

That one was opened a month ago and never returned to. You could merge their changes into yours, or start (over) with their changes/branch and put yours on top, then we can merge giving you both credit.

@mscheltienne
Copy link
Member Author

Failures related to #11454 maybe?

@drammock
Copy link
Member

Failures related to #11454 maybe?

I opened #11476 to track these failures.

@mscheltienne
Copy link
Member Author

It should be good to merge. Thanks for the fix in #11478!

@drammock
Copy link
Member

axes param is documented twice
https://output.circle-artifacts.com/output/job/b44bcf3f-92ce-4799-95b2-7979266997d8/artifacts/0/dev/generated/mne.viz.plot_montage.html#mne-viz-plot-montage

TBH I would have expected that to raise an error :/

larsoner
larsoner reviewed Feb 15, 2023
View reviewed changes
mne/viz/montage.py Outdated Show resolved Hide resolved
@larsoner larsoner enabled auto-merge (squash) February 15, 2023 20:53
@mscheltienne
Copy link
Member Author

Arf.. that's from the merge of #11404 🙈 Thanks @drammock !

@larsoner larsoner merged commit d740e47 into mne-tools:main Feb 15, 2023
@mscheltienne mscheltienne deleted the dev_axes branch February 15, 2023 22:10
larsoner added a commit to drammock/mne-python that referenced this pull request Feb 22, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into test-mpl-fix
c065743 
* upstream/main: (46 commits)
  Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499)
  Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473)
  MAINT: Fix Circle [circle deploy] (mne-tools#11497)
  MAINT: Use mamba in CIs (mne-tools#11471)
  Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479)
  Fix typo in tutorial (mne-tools#11498)
  Typo fix and added colons before code (mne-tools#11496)
  [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493)
  Accept only left-clicks for adding annotations (mne-tools#11491)
  [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489)
  [ENH] Added unit_role to add non-breaking space between magnitude  and units (mne-tools#11469)
  MAINT: Fix CircleCI build (mne-tools#11488)
  [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475)
  Typo fix (mne-tools#11485)
  [MRG] Forward argument axes from plot_sensors to DigMontage.plot (mne-tools#11470)
  [MRG] Improve error message raised on channels missing from DigMontage (mne-tools#11472)
  MAINT: Deal with pkg_resources usage bugs (mne-tools#11478)
  Add object array support and docstring (mne-tools#11465)
  [ENH] Adjusted SSD algorithm to support non-full rank data (mne-tools#11458)
  [BUG] fix nibabel reference (mne-tools#11467)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mscheltienne @larsoner @drammock @janEbert