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

Remove recursion in plot_ica_components and use context manager for plt.ion/plt.ioff #11696

Merged
merged 8 commits into from
May 16, 2023
Merged

Remove recursion in plot_ica_components and use context manager for plt.ion/plt.ioff #11696

merged 8 commits into from
May 16, 2023

Conversation

mscheltienne
Copy link
Member

@mscheltienne mscheltienne commented May 15, 2023 •
edited
Loading

Closes #11693
Closes #11510 (lucky hit, that was the same bug at play)

Prevents spyder console and jupyter notebook from hanging because of:

plt.ion()
# stuff
plt.ioff()

And restructure plot_ica_components to remove recursion and gather all figures before plt.show.

@mscheltienne mscheltienne marked this pull request as draft May 15, 2023 12:40
@mscheltienne mscheltienne mentioned this pull request May 15, 2023
Closed
mscheltienne
mscheltienne commented May 15, 2023
View reviewed changes
mne/viz/_mpl_figure.py Outdated
Comment on lines 78 to 85
plt.ion()
BACKEND = get_backend()
# This ↑↑↑↑↑↑↑↑↑↑↑↑↑ does weird things:
# https://github.com/matplotlib/matplotlib/issues/23298
# but wrapping it in ion() context makes it go away (can't actually use
# `with plt.ion()` as context manager, though, for compat reasons).
# Moving this bit to a separate function in ../../fixes.py doesn't work.
plt.ioff()
Copy link
Member Author

@mscheltienne mscheltienne May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That part is what is hanging jupyter notebooks and spyder consoles.

Copy link
Member

@larsoner larsoner May 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the right thing to do here would be to take advantage of the context-manager capabilities (available in 3.4 already which is our min req, probably wasn't the case when this comment was written as context-manager support isn't available in 3.3) like:

with plt.ion():
    BACKEND = get_backend()

Copy link
Member

Choose a reason for hiding this comment

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

... and if you could please do this in mne/viz/backends/_notebook.py as well, it would be much appreciated! (Found via git grep when looking into this issue in our repo...)

@larsoner larsoner added the backport-candidate on-merge: backport to maint/1.7 label May 15, 2023
@larsoner
Copy link
Member

Labeling as backport candidate since I was hitting this issue before the 1.4 cut and the min version that supports context managing was listed as mpl 3.4 for MNE 1.4 👍

@mscheltienne
Copy link
Member Author

Done, good point for the context manager, glad we can use it 😄
For the minimum matplotlib version, it should also be specified in requirements.txt, requirements_base.txt and in the conda-forge recipe, no?

@mscheltienne mscheltienne changed the title Remove recursion in plot_ica_components Remove recursion in plot_ica_components and use context manager for plt.ion/plt.ioff May 16, 2023
@mscheltienne mscheltienne marked this pull request as ready for review May 16, 2023 11:47
@larsoner
Copy link
Member

For the minimum matplotlib version, it should also be specified in requirements.txt, requirements_base.txt and in the conda-forge recipe, no?

Yeah we should. Looks like for the feedstock at least we're just out of date:

https://github.com/conda-forge/mne-feedstock/blob/main/recipe/meta.yaml#L63

@larsoner larsoner merged commit 97db26c into mne-tools:main May 16, 2023
@larsoner
Copy link
Member

Awesome, thanks @mscheltienne !

larsoner pushed a commit that referenced this pull request May 16, 2023
@mscheltienne @larsoner
Remove recursion in plot_ica_components and use context manager for p…
2bdad5d 
…lt.ion/plt.ioff (#11696)
@larsoner larsoner added backported and removed backport-candidate on-merge: backport to maint/1.7 labels May 16, 2023
@mscheltienne mscheltienne deleted the plot_ica_topo branch May 16, 2023 18:10
This was referenced May 16, 2023
Merged
Merged
larsoner added a commit to larsoner/mne-python that referenced this pull request May 18, 2023
@larsoner
Merge remote-tracking branch 'upstream/main' into cola5
954a990 
* upstream/main: (32 commits)
  MAINT: Update download buttons [skip azp] [skip actions] [skip cirrus]
  Fix canvas.draw() in callback (mne-tools#11697)
  Remove recursion in plot_ica_components and use context manager for plt.ion/plt.ioff (mne-tools#11696)
  Update affiliation (mne-tools#11695)
  BUG: Fix bug with fwd restriction (mne-tools#11694)
  MRG: Suggest using "conda rename" in MNE updating instructions (mne-tools#11692)
  FIX: Regex [ci skip]
  MAINT: Apply deprecations [circle deploy] (mne-tools#11687)
  MAINT: Release 1.4.0 (mne-tools#11686)
  Trap music (mne-tools#11679)
  Fix call to plot_tfr_topomap from interactive AverageTFR.plot_topo function (mne-tools#11683)
  silence spectrum plot warning in examples/tutorials [circle full] (mne-tools#11682)
  Spectrum plot picks (mne-tools#11680)
  Update website conf (mne-tools#11675)
  BUG: Fix bug with MF LCMV rank (mne-tools#11664)
  ENH: Change known_config_types to dict (mne-tools#11166)
  MAINT: Improve README (mne-tools#11673)
  MAINT: Add to git-blame-ignore-revs [circle front]
  MAINT: Run black on codebase
  MAINT: Use black
  ...
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
backported
Projects
None yet
Milestone
No milestone
2 participants
@mscheltienne @larsoner