Skip to content

Conversation

@SophieHerbst
Copy link
Contributor

@SophieHerbst SophieHerbst commented Feb 14, 2024

Reference issue

Needed to address
mne-tools/mne-bids-pipeline#848

What does this implement/fix?

Allow to pass image parameters when plotting epochs_images in report.add_epochs: vmin and vmax to control the color scale across participants.

@hoechenberger can you have a look?

SophieHerbst and others added 3 commits February 14, 2024 17:19
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
…-python into report_add_image_kwargs

asked to merge upstream before pushing
@hoechenberger
Copy link
Member

hoechenberger commented Feb 14, 2024

@SophieHerbst Almost there, I believe … But linting fails:

mne/report/report.py:1126:89: E501 Line too long (112 > 88)
Found 1 error.

Copy link
Member

@hoechenberger hoechenberger left a comment

Choose a reason for hiding this comment

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

LGTM

@larsoner There's no tests so far, maybe we should add one…

@SophieHerbst
Copy link
Contributor Author

@hoechenberger I don't understand what I have to do about the failed tests.

@hoechenberger
Copy link
Member

Ah. We need to set image_kwargs to an empty dictionary if it's None. Otherwise the **image_kwargs part doesn't work

@hoechenberger
Copy link
Member

@SophieHerbst What I mean is doing something like:

if image_kwargs is None:
    image_kwargs = dict()

:)

@hoechenberger
Copy link
Member

@larsoner Could you take over here please? I'll be busy until the end of the week

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.

Added a tiny test, marking for merge-when-green, thanks @SophieHerbst !

@larsoner larsoner enabled auto-merge (squash) February 16, 2024 14:42
@larsoner larsoner changed the title add image_kwargs to report.add_epochs ENH: Add image_kwargs to report.add_epochs Feb 16, 2024
@larsoner larsoner disabled auto-merge February 16, 2024 14:45
@larsoner larsoner enabled auto-merge (squash) February 16, 2024 14:45
@larsoner
Copy link
Member

Last CircleCI run worked, no idea why this one didn't but I think we can merge!

@larsoner larsoner disabled auto-merge February 16, 2024 16:18
@larsoner larsoner merged commit 5e23fe0 into mne-tools:main Feb 16, 2024
larsoner added a commit to SophieHerbst/mne-python that referenced this pull request Feb 20, 2024
* upstream/main:
  [pre-commit.ci] pre-commit autoupdate (mne-tools#12453)
  MAINT: Fix CIs for PyQt6 (mne-tools#12452)
  DOC: add missing info to interpolate bads docstring (mne-tools#12448)
  FIX: na_rep for reports (mne-tools#12447)
  ENH: Multiple raw instance support to head pos average (mne-tools#12445)
  ENH: Add image_kwargs to report.add_epochs (mne-tools#12443)
snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Richard Höchenberger <richard.hoechenberger@gmail.com>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
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.

3 participants