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.

Sign up for GitHub

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

Jump to bottom

Add color normalization parameter to plot_topomap #9468

Merged
merged 17 commits into from
Jun 24, 2021
Merged

Add color normalization parameter to plot_topomap #9468

merged 17 commits into from
Jun 24, 2021

Conversation

cbrnr
Copy link
Contributor

@cbrnr cbrnr commented Jun 12, 2021 •
edited
Loading

Fixes #9466. Things to do/discuss:

  • I've named the parameter cnorm (for "color normalization"), because I think this better describes its purpose. It also avoids a name clash with an already existing local name norm (which has something to do with the L2 norm, not entirely sure). However, Matplotlib calls this parameter norm, so if we want to mimic their naming scheme we could also call our parameter norm instead.
  • I need to add a test.
  • When passing cnorm, simultaneously passing vmin and vmax is deprecated, because these are already defined in the normalizer. I've implemented a pretty blunt workaround, but we could instead define a default linear normalizer with vmin and vmax and then never pass these explicitly.

drammock
drammock reviewed Jun 14, 2021
View reviewed changes
mne/viz/topomap.py Outdated Show resolved Hide resolved
@drammock
Copy link
Member

we could instead define a default linear normalizer with vmin and vmax and then never pass these explicitly

That sounds slightly nicer to me, but the results should be the same either way (right?). So I'd say do whatever you think leads to easier to understand / cleaner code.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 14, 2021

I think there's an even simpler (but less flexible) option: in addition to vmin and vmax we could also have v0, which by default is right in the middle of the data. If set, it would imply a TwoSlopeNorm (so no other norms would be possible, but maybe that's not even necessary). WDYT?

@drammock
Copy link
Member

I prefer passing a norm rather than passing vmin/vmid/vmax. More flexible, not hard to code on our end, and allows, e.g., logarithmic norms if someone wanted that.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 16, 2021

That sounds slightly nicer to me, but the results should be the same either way (right?). So I'd say do whatever you think leads to easier to understand / cleaner code.

Yes, it's exactly the same. I'll think about it.

Any preference for norm vs. cnorm?

@drammock
Copy link
Member

Any preference for norm vs. cnorm?

slight preference for cnorm for the reasons you mention above.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 18, 2021

I've added a Notes section, and I'm now always using a normalizer internally. If both cnorm and vmin/vmax are passed, I'm issuing a warning – should I perhaps just log as info instead?

@drammock
Copy link
Member

should I perhaps just log as info instead?

Warning is preferred any time we're ignoring user input, I think.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 18, 2021

Any idea what's up with CircleCI (ModuleNotFoundError: No module named 'pyvista.core.filters')?

@drammock
Copy link
Member

Any idea what's up with CircleCI (ModuleNotFoundError: No module named 'pyvista.core.filters')?

no idea, I always ask @GuillaumeFavelier when pyvista fails on the CIs https://app.circleci.com/pipelines/github/mne-tools/mne-python/8624/workflows/daf68146-3366-46a7-b809-1fdf7b221e6f/jobs/29702/steps?invite=true#step-118-71

@larsoner
Copy link
Member

Circle failures will be fixed by pyvista/pyvista#1398

@cbrnr cbrnr marked this pull request as ready for review June 21, 2021 14:47
@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 22, 2021 •
edited
Loading

@GuillaumeFavelier I'm pretty sure the error in Main Notebook are not related to my changes - do you know what's causing this?

@GuillaumeFavelier
Copy link
Contributor

It seems to be related to a recent change in pyvista 0.31.2, I'll push a fix 👍

@GuillaumeFavelier GuillaumeFavelier mentioned this pull request Jun 22, 2021
Merged
@cbrnr cbrnr force-pushed the topomap-norm branch 2 times, most recently from 6444ce3 to d6ac3cb Compare June 22, 2021 16:18
@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 22, 2021

Other than the pip-pre jobs timing out this is ready for review.

@cbrnr cbrnr requested review from drammock and larsoner June 23, 2021 06:20
@larsoner
Copy link
Member

@cbrnr it looks like @drammock has been reviewing so I'll wait for his approval first unless he's busy or there is something specific I could help with

drammock
drammock approved these changes Jun 23, 2021
View reviewed changes
Copy link
Member

@drammock drammock left a comment

Choose a reason for hiding this comment

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

just a nitpick, I think it reads a little clearer now but if you disagree @cbrnr feel free to disregard.

mne/viz/topomap.py Outdated Show resolved Hide resolved
mne/viz/topomap.py Outdated Show resolved Hide resolved
larsoner
larsoner reviewed Jun 23, 2021
View reviewed changes
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.

Just a few minor things from me, otherwise LGTM

mne/viz/topomap.py Show resolved Hide resolved
mne/viz/topomap.py Outdated Show resolved Hide resolved
mne/viz/topomap.py Show resolved Hide resolved
mne/viz/topomap.py Outdated Show resolved Hide resolved
mne/viz/topomap.py Show resolved Hide resolved
cbrnr and others added 12 commits June 24, 2021 11:45
@cbrnr
Add versionadded tag
3ce0f16
@cbrnr
Indent versionadded
c5a78d6
@cbrnr
Add changelog entry
ab83cc2
@cbrnr
Make test work with older matplotlib versions
3989930
@cbrnr
Use old-style numpy RNG
7c4847d
@cbrnr @drammock
Update mne/viz/topomap.py
cb2c319 
Co-authored-by: Daniel McCloy <dan@mccloy.info>
@larsoner @cbrnr
FIX: Intersphinx
604ab3f
@cbrnr @larsoner
Add versionadded tag
6c294bf 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@cbrnr @larsoner
Update mne/viz/topomap.py
6f9a1b6 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@cbrnr @larsoner
Type-check
f4aa9f1 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@cbrnr @larsoner
Move import to top of function
c62640e 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@cbrnr @larsoner
Update mne/viz/topomap.py
7552240 
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 24, 2021

Any idea what's up with CircleCI?

E: Failed to fetch http://deb.debian.org/debian/pool/main/g/graphviz/libpathplan4_2.40.1-6_amd64.deb  404  Not Found [IP: 151.101.250.132 80]

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 24, 2021

Seems like this file doesn't exist: http://deb.debian.org/debian/pool/main/g/graphviz/

There's a file called libpathplan4_2.40.1-6+deb10u1_amd64.deb - maybe the package cache doesn't get refreshed?

@drammock
Copy link
Member

hopefully just a temporary internet snafu, I restarted it. If I paste the link into a browser tab it does prompt me to download a deb file.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 24, 2021

Yes, you're right, it does download the file locally.

@larsoner
Copy link
Member

There is a fix in #9484 (sudo apt-get update) which I'm going to merge imminently.

@cbrnr
Copy link
Contributor Author

cbrnr commented Jun 24, 2021

Finally all CIs are green! @larsoner feel free to merge.

@larsoner larsoner merged commit b6a3608 into mne-tools:main Jun 24, 2021
@larsoner
Copy link
Member

Thanks @cbrnr !

@cbrnr cbrnr deleted the topomap-norm branch June 24, 2021 18:27
alexrockhill pushed a commit to larsoner/mne-python that referenced this pull request Jun 25, 2021
@cbrnr @drammock
@larsoner @alexrockhill
Add color normalization parameter to plot_topomap (mne-tools#9468)
ee1a866 
* Add color normalization parameter to plot_topomap

* Add Notes

* Warn if both cnorm and vmin/vmax are passed

* Add test

* Add versionadded tag

* Indent versionadded

* Add changelog entry

* Make test work with older matplotlib versions

* Use old-style numpy RNG

* Update mne/viz/topomap.py

Co-authored-by: Daniel McCloy <dan@mccloy.info>

* FIX: Intersphinx

* Add versionadded tag

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

* Update mne/viz/topomap.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

* Type-check

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

* Move import to top of function

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

* Update mne/viz/topomap.py

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>

Co-authored-by: Daniel McCloy <dan@mccloy.info>
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@cbrnr cbrnr mentioned this pull request Oct 6, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larsoner larsoner larsoner left review comments

@drammock drammock drammock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add norm parameter to plot_topomap
4 participants
@cbrnr @drammock @larsoner @GuillaumeFavelier