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.

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

[BUG] Evoked.plot_topomap() and Evoked.plot_joint() handle the colormaps in a suboptimal manner #13050

Closed
ruuskas opened this issue Jan 8, 2025 · 10 comments · Fixed by #13063
Labels

Comments

@ruuskas
Copy link
Contributor

ruuskas commented Jan 8, 2025

Description of the problem

This might be somewhat controversial as a bug report and is partially a matter of taste.

There is a combination of two issues, which leads to suboptimal results when producing plots with the default parameters using Evoked.plot_topomap() and Evoked.plot_joint().

Essentially, an unpredictable amount of whitespace is left above and below the colored portion of the colorbar when using Evoked.plot_topomap(). This is seen in all MNE examples using topomaps, for example, Plotting topographic maps of evoked data. The reason is that the colorbar ticks follow the contours (as they should), while the colored part is dependent on vlim (by default set symmetrically such that vmax = np.abs(data).max()). The default behavior is to set six contours using matplotlib.ticker.MaxNLocator, which adds locations beyond the minimum and maximum of the data to obtain "nice" values for the ticks. I wonder if it would be better to scale then also the colormap limits to match the smallest and largest ticks?

This issue can, of course, be circumvented by explicitly setting both vlim and contours.

Moreover, when using Evoked.plot_joint() without specifying vlim, the vlim for the topomaps are selected based on the data at the selected time points (inside plot_evoked_topomap). At the same time, the contours, and correspondingly, the colorbar ticks, are selected based on the ylim of the time series plot. In my opinion, it would be better to do the following:

  • If vlim are specified for the topomap, use those vlim also to set the contours (unless contours are set explicitly). The current behavior is to use the ylim from the time series plot to determine the contours unless contours is set explicitly.
  • If vlim are not specified for the topomap, set the contours similar to what is now done in Evoked.plot_topomap(), using vmax = np.abs(data).max() at the topomap timepoints.

Steps to reproduce

"""
Minimal working example based on the tutorial "Plotting topographic maps of evoked data".
"""

# %%
import numpy as np

from mne import read_evokeds
from mne.datasets import sample

path = sample.data_path()
fname = path / "MEG" / "sample" / "sample_audvis-ave.fif"

# load evoked corresponding to a specific condition
# from the fif file and subtract baseline
condition = "Left Auditory"
evoked = read_evokeds(fname, condition=condition, baseline=(None, 0))

# %%

times = np.arange(0.05, 0.151, 0.02)
evoked.plot_topomap(times, ch_type="mag")

# %%

evoked.plot_joint(picks='mag')

Link to data

No response

Expected results

I would like it to do this:
evoked_joints
evoked_topomap

Actual results

Instead this happens:

evoked_joints

Here's an example of what happens when suboptimal time points are picked:
evoked_joints

Additional information

I don't know if this is a feature or a bug, but here's to open discussion. I can make a PR if this is something that you want to change.

@ruuskas ruuskas added the BUG label Jan 8, 2025
Copy link

welcome bot commented Jan 8, 2025

Hello! 👋 Thanks for opening your first issue here! ❤️ We will try to get back to you soon. 🚴

@larsoner
Copy link
Member

larsoner commented Jan 8, 2025

This is seen in all MNE examples using topomaps, for example

I had seen some colorbar badness and assumed it was a matplotlib bug, it would be great if working this out fixed it!

I wonder if it would be better to scale then also the colormap limits to match the smallest and largest ticks? ... In my opinion, it would be better to do the following:

To build this out a bit more, I think from a user perspective the most intuitive behavior would be:

  1. If you don't pass vlim or contours, MNE sets vlim using max(abs(...)) then chooses contours within those bounds
  2. If you pass vlim but not contours, MNE chooses contours to be within the vlim
  3. If you pass contours but not vlim, MNE chooses vlim to encompass the contours and max(abs(...))
  4. If you pass both contours and vlim, MNE uses them as-is (and if this makes the colorbar have gaps, that's okay/correct)

WDYT?

@drammock
Copy link
Member

drammock commented Jan 8, 2025

  1. If you don't pass vlim or contours, MNE sets vlim using max(abs(...)) then chooses contours within those bounds

  2. If you pass vlim but not contours, MNE chooses contours to be within the vlim

  3. If you pass contours but not vlim, MNE chooses vlim to encompass the contours and max(abs(...))

  4. If you pass both contours and vlim, MNE uses them as-is (and if this makes the colorbar have gaps, that's okay/correct)

case (3) could end up looking really weird if the user's most extreme chosen contours are far from max(abs(data)). If most extreme contours << max(abs(data)) you end up with a colorbar with all ticks clustered at the middle of the color range, or (if most extreme contours >> max(abs(data)) you get a nice looking colorbar + ticks but the topoplot itself only shows values near the center of the color range (AKA whiteish). But arguably either one of those outcomes is the user's fault for choosing contour values poorly so I guess I'm OK with it.

@larsoner
Copy link
Member

larsoner commented Jan 8, 2025

But arguably either one of those outcomes is the user's fault for choosing contour values poorly so I guess I'm OK with it.

Yeah that's my feeling in that case. And they can work around it easily with (4) if they want.

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 13, 2025

Sorry for the delay, I forgot to reply after checking the comments last week.

1. If you don't pass `vlim` or `contours`, MNE sets `vlim` using `max(abs(...))` then chooses contours within those bounds

2. If you pass `vlim` but not `contours`, MNE chooses `contours` to be within the `vlim`

3. If you pass `contours` but not `vlim`, MNE chooses `vlim` to encompass the `contours` and `max(abs(...))`

4. If you pass both `contours` and `vlim`, MNE uses them as-is (and if this makes the colorbar have gaps, that's okay/correct)

WDYT?

This sounds good to me. The only possible problem I see here is that in case (1), it is possible that the contours with the largest absolute values might be quite a bit smaller than vlim depending on the data. This would mean that the colorbar would not show the actual extent of the data (since it is capped at the smallest and largest contours).

For the implementation of the above, I would probably use matplotlib.ticker.MaxNLocator as before but simply discard the first and last ticks, which are outside the specified vlim bounds.

@larsoner
Copy link
Member

This would mean that the colorbar would not show the actual extent of the data (since it is capped at the smallest and largest contours).

Perhaps the colorbar limits in all cases should be capped wherever the vlim limits are

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 13, 2025

This would mean that the colorbar would not show the actual extent of the data (since it is capped at the smallest and largest contours).

Perhaps the colorbar limits in all cases should be capped wherever the vlim limits are

That could be one solution. In that case, there wouldn't be ticks at the extrema, since vlim are usually not "nice" numbers unless specified by the user.

@larsoner
Copy link
Member

I think that's okay. That's often the case for matplotlib plots so perhaps expected behavior

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 13, 2025

Ok. I can implement that and see if it works as expected.

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 14, 2025

Let's continue this in #13063

qian-chu pushed a commit to qian-chu/mne-python that referenced this issue Jan 20, 2025
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <[email protected]>
larsoner added a commit to larsoner/mne-python that referenced this issue Jan 24, 2025
* upstream/main: (57 commits)
  Allow lasso selection sensors in a plot_evoked_topo (mne-tools#12071)
  MAINT: Fix doc build (mne-tools#13076)
  BUG: Improve sklearn compliance (mne-tools#13065)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13073)
  MAINT: Add Numba to 3.13 test (mne-tools#13075)
  Bump autofix-ci/action from ff86a557419858bb967097bfc916833f5647fa8c to 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef in the actions group (mne-tools#13071)
  [BUG] Correct annotation onset for exportation to EDF and EEGLAB (mne-tools#12656)
  New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037)
  [BUG] Fix taper weighting in computation of TFR multitaper power (mne-tools#13067)
  [FIX] Reading an EDF with preload=False and mixed frequency (mne-tools#13069)
  Fix evoked topomap colorbars, closes mne-tools#13050 (mne-tools#13063)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13060)
  BUG: Fix bug with interval calculation (mne-tools#13062)
  [DOC] extend documentation for add_channels (mne-tools#13051)
  Add `combine_tfr` to API (mne-tools#13054)
  Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058)
  BUG: Fix bug with helium anon (mne-tools#13056)
  [ENH] Add option to store and return TFR taper weights (mne-tools#12910)
  BUG: viz plot window's 'title' argument showed no effect. (mne-tools#12828)
  MAINT: Ensure limited set of tests are skipped (mne-tools#13053)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants