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

Fix lingering topomap api #11371

Merged
merged 28 commits into from Dec 17, 2022
Merged

Conversation

drammock
Copy link
Member

closes #11368

Highlights:

  • changes are made to CSP.plot_patterns(), CSP.plot_filters(), ICA.plot_components() and plot_ica_components() to standardize their API with other topomap-plotting functions/methods (follow-up to Standardize topomap args #11123)
  • for all 4 of them, vmin/vmax deprecated and replaced with vlim, as in Standardize topomap args #11123
  • for all 4 of them, new parameters extrapolate, border, cnorm, axes, nrows, ncols. For the ICA-components func/meth, they also gained show_names, size, cbar_fmt.
  • unlike all the other topomap-plotting funcs/meths, the ICA-components func/meth retains the title parameter because we were actually setting a default title even when title=None

@larsoner
Copy link
Member

Looks reasonable so far!

@drammock
Copy link
Member Author

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:
main

Here it is on this PR:
local_build

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.

@larsoner
Copy link
Member

Agreed this looks like a bug fix

@larsoner
Copy link
Member

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!

@larsoner larsoner marked this pull request as ready for review December 16, 2022 12:10
@larsoner larsoner enabled auto-merge (squash) December 16, 2022 12:10
@drammock
Copy link
Member Author

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.

@larsoner
Copy link
Member

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

@drammock
Copy link
Member Author

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.

@larsoner larsoner merged commit 715e979 into mne-tools:main Dec 17, 2022
@drammock drammock deleted the fix-lingering-topomap-api branch December 20, 2022 22:51
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.

finish topomap API consistency
3 participants