Skip to content

Conversation

@hoechenberger
Copy link
Member

Fixes #9812

I don't think we need a changelog entry for this one.

cc @larsoner

@hoechenberger hoechenberger changed the title Allow _plot_mri_contours() to return arrays MRG: Allow _plot_mri_contours() to return arrays Oct 8, 2021
@hoechenberger
Copy link
Member Author

mne/viz/misc.py Outdated
Comment on lines 403 to 404
fig, axs, _, _ = _prepare_trellis(len(slices), n_col)
plt.close(fig) # we'll create a figure for each slice
Copy link
Member

Choose a reason for hiding this comment

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

Why even create this fig, axs at all when the next line is plt.close(fig)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, this line was copied by mistake, shouldn't be here

mne/viz/misc.py Outdated
Comment on lines 313 to 317
slices_as_subplots : bool
Whether to add all slices as subplots to a single figure, or to
create a new figure for each slice.
fig_kind : 'figure' | 'array'
Whether to return Matplotlib figures or NumPy arrays.
Copy link
Member

Choose a reason for hiding this comment

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

I'd assume YAGNI on two parameters here and just keep it as we had it with one. We either want a single figure with multiple panels as the output (i.e., to be shown), or a list of ndarray (i.e., for reports or other embedding).

@hoechenberger
Copy link
Member Author

CI failures seem unrelated.

@agramfort
Copy link
Member

does it help already for something? it helps memory somewhere?

@larsoner
Copy link
Member

larsoner commented Oct 8, 2021

does it help already for something? it helps memory somewhere?

Before the major Report refactoring that was just merged, it was done figure-by-figure for memory reasons IIRC, yeah. So that PR had a small regression in that respect. This PR Should fix it.

@larsoner
Copy link
Member

larsoner commented Oct 8, 2021

(In current main if you don't decimate your MRI you end up with 256 matplotlib figures...)

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.

+1 for MRG when green

thx @hoechenberger

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.

CI failures are mayavi-related deprecation warnings:

E   DeprecationWarning: In the future, a default value of type 'dict' in an Any trait will be shared between all instances. To keep the current semantics, replace `Any({})` with `Any(factory=dict)` or `Any({1: 2})` (for example) with `Any(factory=dict, args=({1: 2},))`.

seems unrelated.

@hoechenberger hoechenberger merged commit f3f0e08 into mne-tools:main Oct 8, 2021
@hoechenberger hoechenberger deleted the hoechenberger/issue9812 branch October 8, 2021 14:44
larsoner added a commit to rob-luke/mne-python that referenced this pull request Oct 11, 2021
* upstream/main:
  MAINT: Update broken link, fix rendering (mne-tools#9829)
  Ensure plot_ica_sources() always plots traces of rejected ICs on top (mne-tools#9823)
  Improve plot_ica_sources() docstring (mne-tools#9825)
  MRG: Fix docstring for plot_ica_components() (mne-tools#9826)
  unpin jsonschema and filter its warning instead (mne-tools#9822)
  Add warning for SNIRF files with != 2 wavelengths (mne-tools#9817)
  add show_scalebars param to epochs.plot() (mne-tools#9815)
  MRG: Allow _plot_mri_contours() to return arrays (mne-tools#9818)
  MRG: Expand ~ in _check_fname() (mne-tools#9613)
  Improve ICA.plot_overlay() docstrings (mne-tools#9820)
  WIP, MAINT: Fix CircleCI (again) (mne-tools#9814)
  MRG, ENH: Add options to fit_dipole (mne-tools#9810)
  Rework Reports (new history) (mne-tools#9754)
  MRG, CI: Use VTK pre (mne-tools#9803)
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.

BUG: _plot_mri_contours makes too many figures

4 participants