Looks reasonable so far!

apparently sometime today GitHub actions deprecated the 18.04 ubuntu image, so our compat-old job is failing.

compat / old: .github#L1
The ubuntu-18.04 environment is deprecated, consider switching to ubuntu-20.04(ubuntu-latest), or ubuntu-22.04 instead. For more details see actions/runner-images#6002

That aside, there is one difference between this PR and main that I want a second pair of eyes on. It's the ica.plot_properties() fig in our tutorial. Pay special attention to the second component (ICA001).

Here it is on main:

Here it is on this PR:

I think that it's actually correct on this PR and was incorrect on main. My reasoning is based on this:

import numpy as np
import mne
from mne.preprocessing import (ICA, create_eog_epochs, create_ecg_epochs,
                               corrmap)

sample_data_folder = mne.datasets.sample.data_path()
sample_data_raw_file = (sample_data_folder / 'MEG' / 'sample' /
                        'sample_audvis_filt-0-40_raw.fif')
raw = mne.io.read_raw_fif(sample_data_raw_file)
raw.crop(tmax=60.).pick_types(meg='mag', eeg=True, stim=True, eog=True)
raw.load_data()
filt_raw = raw.copy().filter(l_freq=1., h_freq=None)
ica = ICA(n_components=15, max_iter='auto', random_state=97)
ica.fit(filt_raw)

picks = np.arange(15)
data = np.dot(ica.mixing_matrix_[:, picks].T,
              ica.pca_components_[:ica.n_components_])
print(data[1].min(), data[1].max())
# -0.016085387042690098 0.8193667881245026

notice that the data for that component straddles zero, and so should have some (dark) red and some (not very dark) blue, but on main it is all red.

Agreed this looks like a bug fix

Failure looks real:

___________________________ test_plot_ica_components ___________________________
mne/viz/tests/test_ica.py:68: in test_plot_ica_components
    ica.plot_components(components, image_interp='cubic',
mne/preprocessing/ica.py:2194: in plot_components
    return plot_ica_components(
<decorator-gen-99>:12: in plot_ica_components
    ???
mne/viz/topomap.py:1312: in plot_ica_components
    cbar.set_ticks(vlim)
/usr/share/miniconda/envs/mne/lib/python3.10/site-packages/matplotlib/_api/deprecation.py:384: in wrapper
    return func(*inner_args, **inner_kwargs)
/usr/share/miniconda/envs/mne/lib/python3.10/site-packages/matplotlib/colorbar.py:878: in set_ticks
    self._long_axis().set_ticks(ticks, labels=labels, minor=minor,
/usr/share/miniconda/envs/mne/lib/python3.10/site-packages/matplotlib/axis.py:2030: in set_ticks
    result = self._set_tick_locations(ticks, minor=minor)
/usr/share/miniconda/envs/mne/lib/python3.10/site-packages/matplotlib/axis.py:1987: in _set_tick_locations
    axis.set_view_interval(min(ticks), max(ticks))
E   TypeError: '<' not supported between instances of 'NoneType' and 'NoneType'

@drammock feel free to fix, I've marked as merge-when-green. Then hopefully we can release!

OK so here's what's going on with ica.plot_components():

• that second component is in fact all positive (data lims=(0.264, 0.819))
• we need to run _setup_vmin_vmax() before the internal call to plot_topomap() because those vlim values get used for colorbar ticks, in cases where each topomap has a colorbar.
• that call to _setup_vmin_vmax() was not getting passed a norm boolean, so it was always returning 2-sided symmetrical vlims.
  • given the all-positive component data, we get an all-red topomap
• Since the vlims are now floats (and not None), the later call to _setup_vmin_vmax() (inside of _plot_topomap()) is a pass-through so the value of norm computed there and passed to it is ignored.

Proposed solution:

keep it like current main (each map gets its own vmin/vmax, and all the colormaps are forced to be symmetrical about zero even if their data is all-positive). I think it makes sense given the nature of the ICA decomposition: all components are at least potentially both positive and negative (unlike some topomap data like field power) so forcing symmetric vlims onto one-sided data actually makes sense here. Also it appears to have been this way since 0.22 at least, so it won't surprise anyone.

Ultimately I'd vlim='joint' like we have for projector topomaps. I think it makes sense for ICA components too, but maybe is not a good default. Anyway that can be a separate PR.

Sounds good to me, feel free to mark for merge when your plan has been implemented @drammock unless you want me to take another look

it's already implemented :) but now that compat/old is actually running, it's revealed a new test failure (I'm guessing due to old MPL API). I'll fix it shortly.