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

[MRG] Fixed group_by titles in evoked_plot. #10618

Merged
merged 3 commits into from May 12, 2022
Merged

[MRG] Fixed group_by titles in evoked_plot. #10618

merged 3 commits into from May 12, 2022

Conversation

rezashr
Copy link
Contributor

@rezashr rezashr commented May 9, 2022

Fixes #10608

This fixes the issue with the wrong title of sub-plots.

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.

hey @rezashr could you explain how this fixes the issue?

@rezashr
Copy link
Contributor Author

rezashr commented May 11, 2022

The issue resides in the code for selecting an appropriate title:

titles = ({channel_type(evoked.info, idx): sel
for idx in group_by[sel]} if titles is None else titles)

When titles=None, the value was getting changed in the first loop. This fix avoids changing the original titles variable inside the loop.

Maybe show_channels argument can also be improved to take an integer to show labels of a fixed number of electrodes within each sub-plot. Currently it can only be a logical value or 'all'.

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.

Thanks!

I just tested the fix via python -ui tutorials/stats-sensor-space/20_erp_stats.py

and it works:

image

@rezashr could you please:

  1. fix the codespell error in the CI
  2. add your fix to the changelog (see instructions)

Maybe show_channels argument can also be improved to take an integer to show labels of a fixed number of electrodes within each sub-plot. Currently it can only be a logical value or 'all'.

I suggest you open a new issue on that and target it with a dedicated PR if other people agree that this would be helpful :-)

@rezashr
Copy link
Contributor Author

rezashr commented May 12, 2022

@sappelhoff, I did the contribution part, fixed the codespell_and_flake ci, and resolved a small conflict with the main branch.

But a CI failed again with an error related to OpenGL graphics: vtkOpenGLRenderWindow.c:493 ERR| vtkEGLRenderWindow (0x563b392594a0): GLEW could not be initialized: Missing GL version. Do you know how to pass this one?

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.

No idea what that error is. I tried re-running the CI, let's see what happens. Otherwise I am +1 to merge this - thanks!

@sappelhoff sappelhoff changed the title Fixed group_by titles in evoked_plot. [MRG] Fixed group_by titles in evoked_plot. May 12, 2022
@larsoner larsoner merged commit 97bce19 into mne-tools:main May 12, 2022
@larsoner
Copy link
Member

Failure is unrelated and is being taken care of in #10628 so I put this one in, thanks @rezashr !

@rezashr rezashr deleted the fix_plot_evoked_group_by_subtitle branch May 12, 2022 21:46
larsoner added a commit to HanBnrd/mne-python that referenced this pull request May 17, 2022
* upstream/main: (24 commits)
  Use less memory when loading EDF file (mne-tools#10638)
  BUG: fix ipython console accessibility after MNEQtBrowser in Spyder (mne-tools#10637)
  WIP: Fix mne.time_frequency.multitaper Nyquist adjustment slightly incorrect (mne-tools#10541)
  BUG: Fix bug with fNIRS reordering (mne-tools#10630)
  MNT: PyQt6 for pip pre 3.10 (mne-tools#10636)
  CI: Fix conda (mne-tools#10628)
  MRG: Update installer links to point to 1.0.3_0 (mne-tools#10622)
  [BUG, MRG] fix info write access (mne-tools#10626)
  [MRG] Fixed group_by titles in evoked_plot. (mne-tools#10618)
  [BUG, MRG] Replace image_interp with interpolation in topomap plot arguments (mne-tools#10617)
  MAINT: Fix timeout (mne-tools#10624)
  Simplify mne-tools#10619 (mne-tools#10620)
  Drop EEG rejection thresholds when replacing EEG with CSD channels (mne-tools#10619)
  Add get_montage support for fNIRS (mne-tools#10611)
  ENH: Improve make_head_surface options (mne-tools#10592)
  [ENH, MRG] Add citation for intracranial JOSS paper (mne-tools#10616)
  [ENH] Add sys info for mne-icalabel (mne-tools#10615)
  MRG: Add "highlight" parameter to Evoked.plot() to conveniently highlight time periods (mne-tools#10614)
  MRG: Allow to pass array of "average" values to plot_evoked_topomap() (mne-tools#10610)
  fix: snirf length units (mne-tools#10613)
  ...
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.

Viz: evoked.plot_image with the group_by parameter produces faulty sub-panel titles.
3 participants