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

Fix evoked topomap colorbars, closes #13050 #13063

Merged
merged 17 commits into from
Jan 16, 2025

Conversation

ruuskas
Copy link
Contributor

@ruuskas ruuskas commented Jan 14, 2025

This should fix #13050 as discussed there.

It now works like this:

  1. If you don't pass vlim or contours, MNE sets vlim using max(abs(...)) then chooses contours within those bounds
  2. If you pass vlim but not contours, MNE chooses contours to be within the vlim
  3. If you pass contours but not vlim, MNE chooses vlim to encompass the contours and max(abs(...))
  4. If you pass both contours and vlim, MNE uses them as-is (and if this makes the colorbar have gaps, that's okay/correct)
  1. If you don't pass vlim or contours, MNE sets vlim using max(abs(...)) then chooses contours within those bounds
times = np.arange(0.05, 0.151, 0.02)
fig = evoked.plot_topomap(times, ch_type="mag")

evoked_topomap_case1

  1. If you pass vlim but not contours, MNE chooses contours to be within the vlim
fig = evoked.plot_topomap(times, ch_type="mag", vlim=(-400, 400))

evoked_topomap_case2

  1. If you pass contours but not vlim, MNE chooses vlim to encompass the contours and max(abs(...))
fig = evoked.plot_topomap(times, ch_type="mag", contours=[-600, -300, 0, 300, 600])

evoked_topomap_case3

  1. If you pass both contours and vlim, MNE uses them as-is (and if this makes the colorbar have gaps, that's okay/correct)
fig = evoked.plot_topomap(times, ch_type="mag", 
                          contours=[-600, -300, 0, 300, 600], 
                          vlim=(-400, 400))

evoked_topomap_case4

This also fixes the issue with plot_evoked_joint:

fig = evoked.plot_joint(times=[0.05, 0.15, 0.2], picks='mag')

evoked_joints1

WDYT?

The section "Running the test suite" was labeled as _run_tests although the references are to _run-tests. For some reason, the _run_tests reference points to a similar section in the documentation of statsmodels.
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.

Wow a pretty small diff!

@ruuskas can you make some tiny change to examples/visualization/evoked_topomap.py (can just be a reflow or newline add/remove)? That way CircleCI will build the example (it looks for changed ones by default and only runs those) and we can see the results. Feel free to also touch any other examples that you might know offhand look bad currently and might magically be fixed by this PR. That way we don't have to run them manually on our machines (or worse, wait until the PR lands in main and look at the deployed doc) to see what changes.

Also needs a doc/changes/devel/13064.bugfix.rst mentioning the change.

Can you add the "it now works like this:" 4-point conditional to the Notes section of plot_evoked_topomap? I think it's a good place to make clear the default behavior

@ruuskas ruuskas changed the title Fix evoked topomap colormaps, closes #13050 Fix evoked topomap colorbars, closes #13050 Jan 14, 2025
@ruuskas ruuskas requested a review from agramfort as a code owner January 14, 2025 19:07
@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 15, 2025

It seems that something is broken with the CIs @larsoner @drammock.

@drammock
Copy link
Member

https://app.circleci.com/pipelines/github/mne-tools/mne-python/26410/workflows/72bf256a-8e09-444e-b46e-532f1ec83809/jobs/71029/parallel-runs/0/steps/0-142?invite=true#step-142-83999_21

evoked.plot_topomap docstring has a bullet list that ends without a blank line, according to the warning message

mne/viz/topomap.py Outdated Show resolved Hide resolved
Co-authored-by: Eric Larson <[email protected]>
@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 15, 2025

https://app.circleci.com/pipelines/github/mne-tools/mne-python/26410/workflows/72bf256a-8e09-444e-b46e-532f1ec83809/jobs/71029/parallel-runs/0/steps/0-142?invite=true#step-142-83999_21

evoked.plot_topomap docstring has a bullet list that ends without a blank line, according to the warning message

I see that now. Earlier today the CircleCI run wouldn't start at all.

@larsoner
Copy link
Member

@ruuskas looks good but I think this point above is still pending:

@ruuskas can you make some tiny change to examples/visualization/evoked_topomap.py (can just be a reflow or newline add/remove)? That way CircleCI will build the example (it looks for changed ones by default and only runs those) and we can see the results. Feel free to also touch any other examples that you might know offhand look bad currently and might magically be fixed by this PR. That way we don't have to run them manually on our machines (or worse, wait until the PR lands in main and look at the deployed doc) to see what changes.

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 15, 2025

@ruuskas looks good but I think this point above is still pending:

@ruuskas can you make some tiny change to examples/visualization/evoked_topomap.py (can just be a reflow or newline add/remove)? That way CircleCI will build the example (it looks for changed ones by default and only runs those) and we can see the results. Feel free to also touch any other examples that you might know offhand look bad currently and might magically be fixed by this PR. That way we don't have to run them manually on our machines (or worse, wait until the PR lands in main and look at the deployed doc) to see what changes.

Hmm I did add just a newline in the end but it didn't render the plots. I'll try again.

@larsoner
Copy link
Member

Hmm I did add just a newline in the end but it didn't render the plots. I'll try again.

Hah -- that's a style error, so pre-commit.ci fixed that "error" for you!

@ruuskas
Copy link
Contributor Author

ruuskas commented Jan 15, 2025

Now it seems to have worked and the colorbars look ok.

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.

Thanks @ruuskas

@larsoner larsoner merged commit 087779c into mne-tools:main Jan 16, 2025
30 checks passed
qian-chu pushed a commit to qian-chu/mne-python that referenced this pull request Jan 20, 2025
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric Larson <[email protected]>
larsoner added a commit to larsoner/mne-python that referenced this pull request Jan 24, 2025
* upstream/main: (57 commits)
  Allow lasso selection sensors in a plot_evoked_topo (mne-tools#12071)
  MAINT: Fix doc build (mne-tools#13076)
  BUG: Improve sklearn compliance (mne-tools#13065)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13073)
  MAINT: Add Numba to 3.13 test (mne-tools#13075)
  Bump autofix-ci/action from ff86a557419858bb967097bfc916833f5647fa8c to 551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef in the actions group (mne-tools#13071)
  [BUG] Correct annotation onset for exportation to EDF and EEGLAB (mne-tools#12656)
  New feature for removing heart artifacts from EEG or ESG data using a Principal Component Analysis - Optimal Basis Sets (PCA-OBS) algorithm (mne-tools#13037)
  [BUG] Fix taper weighting in computation of TFR multitaper power (mne-tools#13067)
  [FIX] Reading an EDF with preload=False and mixed frequency (mne-tools#13069)
  Fix evoked topomap colorbars, closes mne-tools#13050 (mne-tools#13063)
  [pre-commit.ci] pre-commit autoupdate (mne-tools#13060)
  BUG: Fix bug with interval calculation (mne-tools#13062)
  [DOC] extend documentation for add_channels (mne-tools#13051)
  Add `combine_tfr` to API (mne-tools#13054)
  Add `combine_spectrum()` function and allow `grand_average()` to support `Spectrum` data (mne-tools#13058)
  BUG: Fix bug with helium anon (mne-tools#13056)
  [ENH] Add option to store and return TFR taper weights (mne-tools#12910)
  BUG: viz plot window's 'title' argument showed no effect. (mne-tools#12828)
  MAINT: Ensure limited set of tests are skipped (mne-tools#13053)
  ...
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.

[BUG] Evoked.plot_topomap() and Evoked.plot_joint() handle the colormaps in a suboptimal manner
3 participants