Fix issue where set ylim parameter gets swapped across channel types in plot_evoked_topo

[ramkpari]

Fixes an issue where ylims could get swapped across meg channel types in plot_evoked_topo.

In the current build, this bug has a 50% of occurring and can be replicated by running the following code back to back .

import numpy as np

import mne

from mne.datasets import sample

data_path = sample.data_path()

fname = data_path + '/MEG/sample/sample_audvis-ave.fif'

# Reading evoked data
condition = 'Left Auditory'
evoked = mne.read_evokeds(fname, condition=condition, baseline=(None, 0),
                          proj=False)

evoked.plot_topo(ylim=dict(mag=[-500, 500], grad=[-10, 10]))

What does this implement/fix?

Implements an extra check to flip the order inside types_used only if required.

Additional information

The specific bug occurs due to the randomness of output from line 717 in mne/viz/topo.py .

types_used = {channel_type(info, ch_names.index(ch))
                      for ch in chs_in_layout}

The particular line has a 50: 50 chance to output either [mag,grad] or [grad,mag] for the provided example.

Line 727 converts it into a list and flips the order regardless of the order of input for types_used . The proposed fix adds an additional check to perform the flip only the list is not in the expected order.

[ramkpari]

In terms of the failed checks, I don't think it's related to any of the changes I've made in this PR..

[agramfort]

any chance we can test this?

[ramkpari]

Running the sample code from above on the main branch, back to back in different ipython instances should showcase the bug as it has an alternating pattern of either appearing .

[agramfort]

what I have in mind is update a test to check that the axes in the figure have the correct ylim for example In [7]: fig, _ = plt.subplots(2, 2) In [8]: for ax in fig.axes: print(ax.get_ylim()) (0.0, 1.0) (0.0, 1.0) (0.0, 1.0) (0.0, 1.0)

…

[jasmainak]

did you check that your fix also works for EEG?

any potential simplification to the code will also be appreciated. Most of the set operations can possibly achieved with list comprehensions as well ... might save you the need to reorder post-hoc. Maybe something like:

chs_in_layout = [ch_name for ch_name in ch_names if ch_name in layout.names]

[ramkpari]

EEG works fine. Looks like there was minor bug introduced from PR #9162 for EEG plots.

Converting the set operation into a list comprehension reduces the randomness of the bug from a 50:50 to a 1 in 5 chance of occurring. The randomness still seems to turn up from setting types_used .

[ramkpari]

what I have in mind is update a test to check that the axes in the figure have the correct ylim for example In [7]: fig, _ = plt.subplots(2, 2) In [8]: for ax in fig.axes: print(ax.get_ylim()) (0.0, 1.0) (0.0, 1.0) (0.0, 1.0) (0.0, 1.0)
…

Let me try adding a test for this !

[ramkpari]

I've tried to simplify the whole zipping and unzipping process involved with ylims in relation to meg data specifically , by removing as much of the zip objects as possible.

[rob-luke]

did you check that your fix also works for EEG?

Sorry to be a pain, but does this work for fNIRS data? Similar to MEG with mag/grad there are multiple types for fNIRS hbo/hbr often in the same raw structure.

See here for example of how to create this data type...

mne-python/mne/preprocessing/nirs/tests/test_nirs.py

Line 335 in 88df35f

raw = RawArray(data, info, verbose=True)

If you're unable to check then let me know and I'll validate in a few weeks.

[ramkpari]

did you check that your fix also works for EEG?

Sorry to be a pain, but does this work for fNIRS data? Similar to MEG with mag/grad there are multiple types for fNIRS hbo/hbr often in the same raw structure.

See here for example of how to create this data type...

mne-python/mne/preprocessing/nirs/tests/test_nirs.py

Line 335 in 88df35f

raw = RawArray(data, info, verbose=True)

If you're unable to check then let me know and I'll validate in a few weeks.

So the fix itself doesn't affect fnirs plotting, but it looks like it has a similar issue of channel swapping arising from the same line of setting types used. For the current PR , I just swapped the limits around if it comes out the wrong way for meg data alone.

For my fNIRS testing, I used a combination of hbo and hbr and noticed this issue. Since there are four possible channel types with fNIRS, I might need some clarification over the combination of channel types possible with fNIRS to implement a similar fix for it.

[ramkpari]

For the different possible fNIRS channel type combinations, I can see hbo and hbr being plotted together from the same raw structure. Can 'fnirs_cw_amplitude', 'fnirs_od'be plotted together and can they be individually used with either hbo or hbr? @rob-luke

[rob-luke]

Thanks for looking in to this!

Can 'fnirs_cw_amplitude', 'fnirs_od'be plotted together and can they be individually used with either hbo or hbr?

Good question. Only hbo and hbr are stored/plotted together. We do not support amplitude and od in the same raw structure. Nor do we support a mix of all 4 types.

[ramkpari]

In that case, the last commit (34a0e20) should deal with similar random swapping across channel types for fNIRS data as well

[ramkpari]

Still, needs a test built for checking ylims from the plotted figure.

[rob-luke]

Cheers! That initial ylim confusion we had during the sprint has really led down a rabbit hole. Thanks for fixing this for the extra types too.

[ramkpari]

Ahh, I think there's still a couple of issues left in plot_evoked_topo related to just ylims. As a matter of fact, I think there's one more specific to fNIRS data that I could use your help on. I've emailed you with a couple of specifics!

[ramkpari]

So I've been trying to build a test for this but can't find a way to unpack the figure object or specifically the axes object from inside the figure object that's returned by plot_evoked_topo.

fig,ax= plot_topo(evoked,ylim=dict(mag=[-10, 10],grad=[-100,100]))

I've tried a couple of approaches like the above with the figure object and even the axes object but it keeps throwing an error saying that it's not iterable, specifically TypeError: cannot unpack non-iterable Figure object.

Any tips on how to unpack the figure object would be helpful!

[jasmainak]

@ramkpari floated the idea of a debug parameter for testing. So you call unified=False if debug=True. I seem to recall we've used this elsewhere for testing.

however, I propose we keep the ball moving with the following actions in order:

1. merge this PR
2. @ramkpari makes the PR to always show bad channel and possibly add exclude parameter. Also ylim=None should use data only from channels being plotted
3. new PR to add a test with debug parameter
4. come back to fNIRS data plotting with @rob-luke . This was broken even before this PR?

how does that sound to you @agramfort ?

[ramkpari]

So digging deeper into the source of randomness, it turns out it was the set comprehension that was the root cause of the randomness. From Python 3.3, the __hash__() values of str and bytes objects are “salted” with an unpredictable random value, So the hashes for set comprehension seems to throw the types_used in random order.

The current fix replaces the set comprehension with a list comprehension and also the set operations with equivalent lambda based list filter operations.

References
https://docs.python.org/3.8/reference/datamodel.html#object.__hash__

https://python-security.readthedocs.io/vuln/hash-dos.html

http://ocert.org/advisories/ocert-2011-003.html

[jasmainak]

great sleuthing @ramkpari

if you can clean up this PR to remove the swapping and use list comprehension wherever possible (instead of lambda filtering), I think it's good to go for me

[jasmainak]

All good by me

I let @larsoner or @agramfort merge

[agramfort]

just a tiny cosmit suggestion.

we give up on a test @jasmainak ?

[ramkpari]

I'm not entirely sure if the ci doc build fails is related to the last commit.

[drammock]

I'm not entirely sure if the ci doc build fails is related to the last commit.

It's being worked on in #9274

[jasmainak]

we give up on a test @jasmainak ?

for now yes. Let's move on and I suggest we revisit this in a later PR

[agramfort]

thanks @ramkpari !