BUG : Fixes Bug where ylim would only be applied to the first channel in plot_evoked_topo#9162
Conversation
…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
sappelhoff
left a comment
There was a problem hiding this comment.
awesome, thanks a lot for the digging and this simple fix
|
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? |
|
actually I'm not sure if it's fully fixed. Try changing the evoked_meg.plot_topo(ylim=dict(grad=[-40, 40], mag=[-2000, 2000]),
scalings=dict(grad=1e13, mag=1e15)) |
|
Looks like the new bug is from the unzipping process. Will test and try to fix it! |
|
Still needs a bit of testing ! |
|
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 ... |
|
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? |
|
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. |
|
Looks like it was a case of mixing the limits up between channel types (within that loop) due to the zipping and unzipping process. |
|
That last commit should deal with previous CI issues ! |
jasmainak
left a comment
There was a problem hiding this comment.
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
|
thx a lot @ramkpari ! |
…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>
Reference issue
Fixed a bug where any ylim input parameters to function
plot_evoked_topowould 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_topoWhat 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 .