Skip to content

Conversation

@cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Jan 15, 2021

The title is out of bounds in figures created by plot_sensors (including montage.plot()).

Before:
before

After:
after

Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

Care to add a changelog entry? :)

@larsoner
Copy link
Member

@drammock feel free to review and merge when you're happy since you've been working in the viz code a lot lately

@cbrnr
Copy link
Contributor Author

cbrnr commented Jan 15, 2021

We could also move the title from the canvas to the window title bar (and maximize the montage plot again). What do you think?
Screen Shot 2021-01-15 at 15 54 52

@hoechenberger
Copy link
Member

We could also move the title from the canvas to the window title bar (and maximize the montage plot again). What do you think?

Me likes!! 😍

@cbrnr cbrnr force-pushed the plot-sensors-title branch from b31b742 to 67fb952 Compare January 15, 2021 15:52
@drammock
Copy link
Member

We could also move the title from the canvas to the window title bar (and maximize the montage plot again).

There is a downside to this, in that window titles don't show up in our tutorial docs. We're not super consistent about this throughout the codebase, but I've been leaning toward using the plot title for generic descriptive stuff like "Power spectral density (magnetometers)" or "sensor positions", and using the window title for the information that is more useful interactively (esp. during preprocessing), like the subject ID. On that basis, I would lean toward leaving this as a plot title rather than a window title.

On the other hand, sensor location plots often get inset into data plots, so it might simplify things to not add axes titles to them by default? In which case putting that info in a window title makes sense (as long as we're careful that it doesn't change the window title when the sensor plot is merely an inset).

@cbrnr
Copy link
Contributor Author

cbrnr commented Jan 15, 2021

In general I agree, but in this case the title "Sensor positions (eeg)" really doesn't contain a lot of helpful information and takes up a lot of space. Either way, let me know how to proceed.

@drammock
Copy link
Member

putting that info in a window title makes sense (as long as we're careful that it doesn't change the window title when the sensor plot is merely an inset)

I think this reasoning wins out for me. Just reading the code, I think currently it will change the figure title even when the sensor plot is merely an inset axes.... yep, just tested with this:

import os
import mne
import matplotlib.pyplot as plt
plt.ion()
fig, ax = plt.subplots()
inax = ax.inset_axes((0.1, 0.1, 0.4, 0.4))

dp = mne.datasets.multimodal.data_path()
raw = mne.io.read_raw(os.path.join(dp, 'multimodal_raw.fif'))
mne.viz.plot_sensors(raw.info, axes=inax)

@cbrnr
Copy link
Contributor Author

cbrnr commented Jan 15, 2021

Is there a way to check whether the axes are the sole axes in a figure or if they are inset?

@drammock
Copy link
Member

drammock commented Jan 15, 2021

Is there a way to check whether the axes are the sole axes in a figure or if they are inset?

I don't think so... I think ax.inset_axes() is ultimately syntactic sugar around fig.add_axes(), so they end up as the same object type. I think the right approach is to only set the figure window title in cases where our plot_sensors() function created the figure in the first place... so you can leverage the axes_was_none variable for that I think?

EDIT: it would be possible to check the length of fig.axes to see if there's only one axes object present, but I think only setting window titles on figure windows that we create is a sensible policy, so I wouldn't bother checking len(fig.axes)

@cbrnr
Copy link
Contributor Author

cbrnr commented Jan 15, 2021

@drammock I think this should work now - can you check?

Co-authored-by: Daniel McCloy <dan@mccloy.info>
@cbrnr
Copy link
Contributor Author

cbrnr commented Jan 15, 2021

@drammock done!

@drammock drammock changed the title Fix title position in plot_sensors MRG, VIZ: Fix title position in plot_sensors Jan 15, 2021
@drammock drammock merged commit 3a83e2c into mne-tools:master Jan 15, 2021
@drammock
Copy link
Member

thanks @cbrnr!

@cbrnr cbrnr deleted the plot-sensors-title branch January 15, 2021 22:34
larsoner added a commit to vpeterson/mne-python that referenced this pull request Feb 25, 2021
* upstream/master: (66 commits)
  MRG, ENH: Add infant template downloader (mne-tools#8738)
  ENH: add reader for NeuroElectrics .nedf files (mne-tools#8734)
  DOC: improve glossary entry about fiducials (mne-tools#8763)
  MRG, ENH: Add Report.add_custom_css (mne-tools#8762)
  BUG, DOC: read_raw_egi didn't support pathlib.Path; update read_raw() docstring (mne-tools#8759)
  Add "dbs" as new channel type (mne-tools#8739)
  MRG, VIZ: Fix title position in plot_sensors (mne-tools#8752)
  MRG: Support for non-FIFF files in Report.parse_folder (mne-tools#8744)
  MRG, VIZ, FIX: sEEG picking in _prepare_topomap_plot() (mne-tools#8736)
  DOC: don't use single letter variable name in _compute_forward (mne-tools#8727)
  WIP: Fix search [skip github] [skip azp] (mne-tools#8742)
  WIP: Compare Beer-lambert to HOMER (mne-tools#8711)
  MRG: bump spyder version (mne-tools#8020)
  FIX anon with IO round trip (mne-tools#8731)
  fix set_bipolar_reference for Epochs (mne-tools#8728)
  WIP: Add width argument, reduce default (mne-tools#8725)
  ENH: Add toggle-all button to Report (mne-tools#8723)
  fix int/float conversion in nicolet header (mne-tools#8712)
  MRG, BUG: Fix Report.add_bem_to_section n_jobs != 1 (mne-tools#8713)
  MRG, DOC: Make "rank" options in docs more accessible (mne-tools#8707)
  ...
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.

4 participants