Skip to content

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

Merged
larsoner merged 2 commits intomne-tools:mainfrom
cjayb:fix_axes3d_case
Jan 29, 2021
Merged

FIX missing Axes3D import in viz._3d._plot_mpl_stc#8811
larsoner merged 2 commits intomne-tools:mainfrom
cjayb:fix_axes3d_case

Conversation

@cjayb
Copy link
Copy Markdown
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
Copy Markdown
Member

jasmainak commented Jan 28, 2021

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

@agramfort
Copy link
Copy Markdown
Member

agramfort commented Jan 29, 2021

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
Copy Markdown
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
Copy Markdown
Member

larsoner commented Jan 29, 2021

@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.12 label Jan 29, 2021
@cjayb
Copy link
Copy Markdown
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
Copy Markdown
Member

larsoner commented Jan 29, 2021

@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
Copy Markdown
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
Copy Markdown
Member

larsoner commented Jan 29, 2021

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 <larson.eric.d@gmail.com>
@larsoner larsoner added backported and removed backport-candidate on-merge: backport to maint/1.12 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