Skip to content

Conversation

@ramkpari
Copy link
Contributor

@ramkpari ramkpari commented Mar 19, 2021

Reference issue

Fixed a bug where any ylim input parameters to function plot_evoked_topo would be ignored. Mentioned here (#8714 (comment))

The bug arose as a result of a zip object being exhausted after being applied to the first channel inside _plot_topo

What does this implement/fix?

Added a temporary list outside the respective for loop to mitigate the zip object from being exhausted in the first iteration .

…ot_evoked_topo

Fixed a bug where any ylim input parameters to function plot_evoked_topo
would be ignored. The bug arose as a result of a zip object being
exhausted after being applied to the first channel inside _plot_topo
@ramkpari ramkpari marked this pull request as ready for review March 19, 2021 15:42
sappelhoff
sappelhoff previously approved these changes Mar 19, 2021
Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

awesome, thanks a lot for the digging and this simple fix

@jasmainak
Copy link
Member

jasmainak commented Mar 19, 2021

This is wonderful. This makes so much more sense now:

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_meg = evoked.copy().pick_types(meg=True, eeg=False)

evoked_meg.plot_topo(ylim=dict(grad=[-40, 40], mag=[-100, 100]),
                     scalings=dict(grad=1e13, mag=1e15))

Is there an example that we could perhaps update? Does this affect any existing example?

@jasmainak
Copy link
Member

jasmainak commented Mar 19, 2021

actually I'm not sure if it's fully fixed. Try changing the ylim for mag to something high. It doesn't affect the plot ...

evoked_meg.plot_topo(ylim=dict(grad=[-40, 40], mag=[-2000, 2000]),
                     scalings=dict(grad=1e13, mag=1e15))

@sappelhoff sappelhoff dismissed their stale review March 19, 2021 17:44

maybe not fixed, see mainaks comment

@ramkpari
Copy link
Contributor Author

Looks like the new bug is from the unzipping process. Will test and try to fix it!

@ramkpari
Copy link
Contributor Author

Still needs a bit of testing !

@jasmainak
Copy link
Member

Still something is weird. Compare:

evoked_meg.plot_topo(ylim=dict(grad=[-0.5, 0.5], mag=[-50, 50]),
                     scalings=dict(grad=1e13, mag=1e15))

and

evoked_meg.plot_topo(ylim=dict(grad=[-0.5, 0.5], mag=[-300, 300]),
                     scalings=dict(grad=1e13, mag=1e15))

I didn't touch the gradiometer limits but they changed too ...

@ramkpari
Copy link
Contributor Author

Yeah, I just tried it and can confirm that the grad limits change too. Maybe it's because the list of lists is being unzipped somewhere else and that's messing it up?

@jasmainak
Copy link
Member

I guess you can set the limits manually above the for loop we were looking at to see if your hypothesis holds true. Try with both the options above.

@ramkpari
Copy link
Contributor Author

ramkpari commented Mar 20, 2021

Looks like it was a case of mixing the limits up between channel types (within that loop) due to the zipping and unzipping process.

@ramkpari
Copy link
Contributor Author

That last commit should deal with previous CI issues !

@ramkpari ramkpari changed the title Fixes Bug where ylim would only be applied to the first channel in plot_evoked_topo BUG : Fixes Bug where ylim would only be applied to the first channel in plot_evoked_topo Mar 20, 2021
@ramkpari ramkpari requested a review from sappelhoff March 20, 2021 21:07
@sappelhoff sappelhoff requested a review from jasmainak March 20, 2021 21:10
Copy link
Member

@jasmainak jasmainak left a comment

Choose a reason for hiding this comment

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

PR looks good to me. Thanks @ramkpari for persevering.

Btw, ideal solution would probably be to remove the zipping from the calling function but I won't insist since it's your first PR and this is already a valuable contribution to make it work so that the data viz makes more sense

@agramfort agramfort merged commit bbcba95 into mne-tools:main Mar 21, 2021
@agramfort
Copy link
Member

thx a lot @ramkpari !

@ramkpari ramkpari deleted the ylim-bug branch March 29, 2021 12:09
@ramkpari ramkpari restored the ylim-bug branch March 29, 2021 12:09
ramkpari added a commit to ramkpari/mne-python that referenced this pull request Mar 29, 2021
agramfort added a commit that referenced this pull request Apr 10, 2021
…in plot_evoked_topo (#9207)

* Fix issue where set ylim parameter gets swapped across channel types

* Fix minor introduced in PR #9162

* Remove set operation to reduce randomness

* Fix style issue

* Removing the zip conversion process

* Additional check to prevent changed to fnirs data

* Add Transpose to nirs data as well

* Restore order for fnirs ylims if flipped

* Added an XXX marker to work on later

Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>

* Coverting set operations to
remove randomness

* Fix style issue

* Removing additional checks

* Converting lambda filters to list comp

* Update changelog with details

* Fix typo in changelog

* Changes to comments

* Fixes style issue

* Update mne/viz/topo.py

Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>

Co-authored-by: Mainak Jas <jasmainak@users.noreply.github.com>
Co-authored-by: Alexandre Gramfort <alexandre.gramfort@m4x.org>
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.

4 participants