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

FIX missing Axes3D import in viz._3d._plot_mpl_stc #8811

Merged
merged 2 commits into from
Jan 29, 2021
Merged

FIX missing Axes3D import in viz._3d._plot_mpl_stc #8811

merged 2 commits into from
Jan 29, 2021

Conversation

cjayb
Copy link
Contributor

@cjayb cjayb commented Jan 28, 2021

Reference issue

Fixes #8810 .

What does this implement/fix?

Added the missing from mpl_toolkits.mplot3d import Axes3

Additional information

Note that I've also modified the same import in plot_head_positions:

from mpl_toolkits.mplot3d import axes3d  # was this
from mpl_toolkits.mplot3d import Axes3  # surely should be this?

I found this while searching for the actual bug, and remembered reading this recently.

@jasmainak
Copy link
Member

how come this was not caught by any of the tests?

@agramfort
Copy link
Member

was not caught because both axes3d and Axes3D are valid imports. Yet only Axes3D is what you want.

agramfort
agramfort approved these changes Jan 29, 2021
View reviewed changes
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

we should also maybe backport this.

I know it was working before so we should see when this was changed / broken.

@larsoner
Copy link
Member

@cjayb can you push a commit to update doc/changes/latest.inc in the bug section? Otherwise LGTM +1 for merge!

@larsoner larsoner added the backport-candidate on-merge: backport to maint/1.7 label Jan 29, 2021
@cjayb
Copy link
Contributor Author

cjayb commented Jan 29, 2021

The Axes3D is very sinister: its import needed to set up 3D backend functionality, but there's no code that actually calls it. Hence the flake8-related ignore comments too.

FYI we were hoping to bypass the need to add pyvista to hnn_core dependencies, yet have a 'pretty picture' in one of the examples. Given that the mpl backend lacks any faculties to add glyphs, we're likely to just comment out the 3d as optional. All we really want is to illustrate a point on the wm surface...

@larsoner
Copy link
Member

@cjayb looks like something went wrong. Sorry if I messed things up by pushing a commit to update latest.inc. If you want I can take your wording and force-push a fix...

@cjayb
Copy link
Contributor Author

cjayb commented Jan 29, 2021

Not sure what happened, go ahead and force-push, I’m offline!

@larsoner larsoner merged commit 5a6688b into mne-tools:main Jan 29, 2021
@larsoner
Copy link
Member

Thanks @cjayb !

larsoner added a commit that referenced this pull request Jan 29, 2021
@cjayb @larsoner
FIX missing Axes3D import in viz._3d._plot_mpl_stc (#8811)
4df9ed8 
* FIX missing Axes3D import in viz._3d._plot_mpl_stc

* DOC: Latest

Co-authored-by: Eric Larson <[email protected]>
@larsoner larsoner added backported and removed backport-candidate on-merge: backport to maint/1.7 labels Jan 29, 2021
@cjayb cjayb deleted the fix_axes3d_case branch January 29, 2021 16:25
larsoner added a commit to adam2392/mne-python that referenced this pull request Feb 1, 2021
@larsoner
Merge remote-tracking branch 'upstream/main' into fixpersyst
a029254 
* upstream/main:
  MRG, ENH: Add warning about bad whitener conditioning (mne-tools#8805)
  MRG, MAINT: Deprecated param and pytest-qt (mne-tools#8808)
  fix mne.viz.plot_topomap with some missing grad in a pair (mne-tools#8817)
  [MRG] Coregistration-GUI:  use *.mff as digitization source (mne-tools#8790)
  FIX: Path
  [MRG] ENH EGI MFF reader:  populate info['dig'] (mne-tools#8789)
  MRG: Improve Brain UX (mne-tools#8792)
  FIX missing Axes3D import in viz._3d._plot_mpl_stc (mne-tools#8811)
  Better error message if configured download folder doesn't exist (mne-tools#8809)
  MRG, ENH: Add support for other formats to browse_raw (mne-tools#8807)
  MRG, BUG: Allow depth > 1 (mne-tools#8804)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort approved these changes

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

Successfully merging this pull request may close these issues.

'matplotlib' backend for stc.plot is broken
4 participants
@cjayb @jasmainak @agramfort @larsoner