Skip to content

Conversation

@drammock
Copy link
Member

@drammock drammock commented Dec 6, 2021

fixes bug introduced in #10087

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM, let's merge once CIs come back happy!

@larsoner
Copy link
Member

larsoner commented Dec 6, 2021

(it would be nice to have a unit test that would have caught this if you can add one without too much effort)

@drammock
Copy link
Member Author

drammock commented Dec 6, 2021

(it would be nice to have a unit test that would have caught this if you can add one without too much effort)

I played around a bit, but actually can't reproduce the error with the Epochs testing dataset (neither epochs nor epochs_full fixtures)

@larsoner
Copy link
Member

larsoner commented Dec 6, 2021

Okay, I took a quick look -- looks like using raw works

@larsoner
Copy link
Member

larsoner commented Dec 6, 2021

@drammock looks like the test_ica.py failure seen on the Notebook tests is real, I can replicate locally with pytest mne/viz/tests/test_ica.py:

mne/viz/tests/test_ica.py:222: in test_plot_ica_sources
    fig = ica.plot_sources(raw)
mne/preprocessing/ica.py:1964: in plot_sources
    return plot_ica_sources(self, inst=inst, picks=picks,
mne/viz/ica.py:90: in plot_ica_sources
    fig = _plot_sources(ica, inst, picks, exclude, start=start, stop=stop,
mne/viz/ica.py:1106: in _plot_sources
    fig._draw_traces()
mne/viz/_mpl_figure.py:1721: in _draw_traces
    if self.mne.ch_selections is None and self.mne.butterfly
E   AttributeError: 'BrowserParams' object has no attribute 'ch_selections'

@larsoner larsoner merged commit a3066f6 into mne-tools:main Dec 6, 2021
@larsoner
Copy link
Member

larsoner commented Dec 6, 2021

Thanks @drammock !

@drammock drammock deleted the fix-butterfly-index branch June 10, 2022 19:30
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.

2 participants