Standardize topomap args #11123

drammock · 2022-08-31T23:17:08Z

Here is progress so far. NOTE: I recommend clicking the "edit" button for this comment and copy-pasting it into a full-screen text editor (and reducing font size as needed) so you can see the whole table at once without line wrapping / scrolling.

Params that already existed for that func/meth marked by ✓.
Newly-added params indicated by ADDED
Deprecated params indicated by DEPRECATED
params not-yet-but-might-be added indicated by YAGNI.
params judged not applicable to that func/meth indicated by n/a

Also the order of params has changed and all funcs/meths now have an * in their signatures after the first few params, making most params keyword-only. The param order (top-to-bottom) and position of the * are as in the table below:

param name	viz.plot_topomap()	Spectrum	Epochs.plot_psd_topomap()	AverageTFR / plot_tfr_topomap	Evoked	Covariance	Projection	inst.plot_projs_topomap()	preprocessing.corrmap()	notes
various	data, pos, *	bands	tmin, tmax, bands	tmin, tmax, fmin, fmax, *	times, *, average	info	info, *		icas, template, threshold, label
ch_type	✓	✓, *	✓, *	✓	✓	✓, *	n/a	✓, * (wasn't used; fixed)	✓, *	Projection objects are one ch_type
various		normalize, agg_fun, dB	proj, method, normalize, agg_fun, dB	baseline, mode	scalings, proj	scalings, proj, noise_cov
sensors	✓	ADDED	ADDED	✓	✓	✓	✓	✓	✓ (wasn't used; fixed)
show_names	DEPRECATED	ADDED	ADDED	ADDED	✓	✓	ADDED	ADDED	ADDED	when we have `Info`
names	✓	n/a	n/a	n/a	n/a	n/a	n/a	n/a	n/a	when we don't have `Info`
mask	✓	ADDED	ADDED	ADDED	✓	✓	YAGNI	YAGNI	YAGNI?	is there a use case for adding a mask to a proj topomap or corrmap?
mask_params	✓	ADDED	ADDED	ADDED	✓	✓	YAGNI	YAGNI	YAGNI?	is there a use case for adding a mask to a proj topomap or corrmap?
contours	✓	ADDED	ADDED	✓	✓	✓	✓	✓	✓
outlines	✓	✓	✓	✓	✓	✓	✓	✓	✓	DEPRECATED "skirt"
sphere	✓	✓	✓	✓	✓	✓	✓	✓	✓
image_interp	✓	ADDED	ADDED	ADDED	✓	✓	✓	✓	ADDED
extrapolate	✓	ADDED	ADDED	ADDED	✓	✓	ADDED	✓	ADDED
border	✓	ADDED	ADDED	ADDED	✓	✓	✓	✓	ADDED
res	✓	ADDED	ADDED	✓	✓	✓	✓	✓	YAGNI	corrmap scales subplots for template vs. matches so better not to expose this
size	ADDED	ADDED	ADDED	✓	✓	✓	✓	✓	YAGNI	corrmap scales subplots for template vs. matches so better not to expose this
cmap	✓	✓	✓	✓	✓	✓	✓	✓	✓
vlim	ADDED	✓	✓	ADDED	ADDED	ADDED	✓	✓	YAGNI for now	necessary when "joint" is a possibility, but for consistency use everywhere
vmin, vmax	DEPRECATED	-	-	DEPRECATED	DEPRECATED	DEPRECATED	-	-	-	replaced by `vlim`
cnorm	✓	ADDED	ADDED	ADDED	ADDED	ADDED	ADDED	ADDED	YAGNI for now
colorbar	YAGNI for now	ADDED	ADDED	✓	✓	✓	✓	✓	YAGNI for now
cbar_fmt	YAGNI for now	✓	✓	✓	✓	✓	ADDED	ADDED	YAGNI for now
unit	-	-	-	DEPRECATED	-	-	-	-	-
units	YAGNI for now	ADDED	ADDED	ADDED	✓	✓	ADDED	ADDED	YAGNI for now
axes	✓	✓	✓	✓	✓	✓	✓	✓	YAGNI	corrmap has unpredictable number of axes
time_unit, time_format	n/a	n/a	n/a	n/a	✓	n/a	n/a	n/a	n/a
title	-	-	-	DEPRECATED	DEPRECATED	DEPRECATED	-	-	-	DEPRECATED nothing automatic here other than providing top margin space
nrows, ncols	n/a	YAGNI for now	YAGNI for now	n/a	✓	n/a	n/a	n/a	n/a	relevant if n_topomaps unknown/large (when user passes `bands` or `times`)
plot	n/a	n/a	n/a	n/a	n/a	n/a	n/a	n/a	✓
show	✓	✓	✓	✓	✓	✓	✓	✓	✓
onselect	✓									TODO
n_jobs	n/a	✓	✓	n/a	n/a	n/a	n/a	n/a	n/a
verbose	YAGNI	✓	✓	YAGNI	YAGNI	✓	YAGNI	YAGNI	✓
method_kw	n/a	n/a	✓	n/a	n/a	n/a	n/a	n/a	n/a

mmagnuski · 2022-09-08T21:15:32Z

mne/utils/docs.py

+    it will be approximated from the coordinates of ``'Oz'``). ``None`` (the
+    default) is equivalent to ``'auto'`` when enough extra digitization points
+    are available, and (0, 0, 0, {HEAD_SIZE_DEFAULT}) otherwise. Currently the
+    head radius does not affect plotting.


Is the text about radius not affecting plotting true? I haven't used plot_topomap for sometime now (I work on iEEG data now), but there was a time when radius affected the plotting. Many of the eeglab .set files I used had "invisible" head outlines because of this, as the channel positions were in mm or cm, but were interpreted by mne as meters (and so the head outline was the size of one channel in the topo plot).

I wonder that too, but haven't had a chance to test it yet.

@mmagnuski update: head radius definitely does affect plotting. I'll remove this statement.

Thanks for confirming this!

drammock · 2022-09-09T22:19:00Z

OMG, the tragedies unearthed by this PR! I've just discovered that [raw|epochs|evoked].plot_projs_topomap() all have a ch_type argument that is silently ignored. raw.plot_projs_topomap(ch_type="foo") just works.

EDIT: fixed in 61a1746

agramfort · 2022-09-10T09:58:13Z

🤦‍♂️

This reverts commit 6011f58.

…-args * upstream/main: Fix mesh display in tutorial (mne-tools#11200) MAINT: Add arm64 CI using CirrusCI (mne-tools#11209) Fix spatial colors (mne-tools#11201) MAINT: Fix CircleCI error (mne-tools#11205) [circle deploy] Add regression-based approach to removing EOG artifacts (mne-tools#11046) [DOC, MRG] Minor documentation improvements and remove glossary entry for array-like (mne-tools#11207) Fix `include_tmax` not considered in `mne.io.Raw.crop` to check `tmax` in bounds (mne-tools#11204) MAINT: Fix notebook backend (mne-tools#11206) MRG: Fix displayed Raw duration in Jupyter notebook (mne-tools#11203) add EpochsSpectrum.average() (mne-tools#11198)

…-args * upstream/main: MAINT: Fix Cirrus caching (mne-tools#11211)

larsoner · 2022-09-28T17:59:55Z

@drammock I pushed a couple commits to deal with #11046 and now everything is green. Feel free to merge if you're happy with my extra commits

larsoner · 2022-09-29T17:18:13Z

You can ignore the windows conda failure, I'm working around it in #11213 and have opened joblib/threadpoolctl#131

larsoner · 2022-09-29T17:18:55Z

... only 4 failures in https://app.circleci.com/pipelines/github/mne-tools/mne-python/16639/workflows/bac464ab-cfb6-45f7-b5bc-372eb801de16/jobs/49697 !

drammock · 2022-09-30T15:53:01Z

OK I've visually inspected all the changed tutorials and fixed a couple inconsistencies between this PR and main. I'm ignoring small things like different amount of spacing between topomap and title. Marking as merge-when-green!

larsoner · 2022-09-30T16:18:32Z

Thanks @drammock !

agramfort · 2022-10-01T07:59:01Z

thanks a ton @drammock 🙏 🙏 🙏 🙏

* upstream/main: Don't insert superfluous newlines in subprocess log messages (mne-tools#11219) purge _get_args helper func (mne-tools#11215) Standardize topomap args (mne-tools#11123) MAINT: Ensure no datasets are downloaded in tests (mne-tools#11213)

* upstream/main: (64 commits) MAINT: Better check (mne-tools#11229) MAINT: Fix link and update instantiation note (mne-tools#11228) BUG: Add estimated fiducials when missing / assumed head coords (mne-tools#11212) Fix tfr db (mne-tools#11223) MAINT: Update link (mne-tools#11222) add CPGRL doc section (mne-tools#11216) Don't insert superfluous newlines in subprocess log messages (mne-tools#11219) purge _get_args helper func (mne-tools#11215) Standardize topomap args (mne-tools#11123) MAINT: Ensure no datasets are downloaded in tests (mne-tools#11213) MAINT: Fix Cirrus caching (mne-tools#11211) Fix mesh display in tutorial (mne-tools#11200) MAINT: Add arm64 CI using CirrusCI (mne-tools#11209) Fix spatial colors (mne-tools#11201) MAINT: Fix CircleCI error (mne-tools#11205) [circle deploy] Add regression-based approach to removing EOG artifacts (mne-tools#11046) [DOC, MRG] Minor documentation improvements and remove glossary entry for array-like (mne-tools#11207) Fix `include_tmax` not considered in `mne.io.Raw.crop` to check `tmax` in bounds (mne-tools#11204) MAINT: Fix notebook backend (mne-tools#11206) MRG: Fix displayed Raw duration in Jupyter notebook (mne-tools#11203) ...

drammock added 8 commits September 1, 2022 14:53

WIP expose all topomap kwargs

4d5a948

simplify docdict sphere

3182aa4

DRY: refactor unit labeling

a62ec64

avoid circular import

98db8d2

refactor to avoid aggregating twice

692f208

refactor one-off middleman helper func

1503800

deprecate show_names (use names instead)

2b524c6

WIP restore show_names where appropriate

e75c922

drammock force-pushed the standardize-topomap-args branch from 687fca8 to e75c922 Compare September 1, 2022 19:59

drammock added 9 commits September 1, 2022 16:54

refactor helper functions to viz/utils.py

5b9476a

raise error earlier

c9722a8

update plot_arrowmap and its test

9d36d32

fix plot_tfr_topomap show_names

1364c72

fix plot_bridged_electrodes show_names (and test)

c5abe4b

partial refactor of topomap plotting tests

af5dbd8

fix part of test

b0c734c

deprecate outlines="skirt"

5d372bb

fix test

6f2446b

mmagnuski reviewed Sep 8, 2022

View reviewed changes

drammock added 7 commits September 8, 2022 17:21

docdict for vlim

481911d

standardize API for cov.plot_topomap()

f1dc3ae

refactor DRY

f764608

standardize API for plot_evoked_topomap()

e786495

fix indentation

c1268ab

fix tests

0b60569

fix another test

2f6df63

drammock added 2 commits September 9, 2022 17:56

make ch_type actually work for plot_projs_topomap

61a1746

docstring/docdict

c4e09da

drammock and others added 6 commits September 22, 2022 18:07

Revert "short-circuit corrmap when appropriate"

8c7da78

This reverts commit 6011f58.

fix vmin/vmax in tutorials [circle full]

01aa734

Merge branch 'main' into standardize-topomap-args [circle full]

2f8ed4b

TST: Full [circle full]

b8fc6b7

FIX: Update regress

fa11476

larsoner force-pushed the standardize-topomap-args branch from 930b216 to fa11476 Compare September 28, 2022 16:12

Merge remote-tracking branch 'upstream/main' into standardize-topomap…

731affb

…-args * upstream/main: MAINT: Fix Cirrus caching (mne-tools#11211)

larsoner approved these changes Sep 28, 2022

View reviewed changes

drammock added 2 commits September 28, 2022 14:29

[circle full]

1cfb51a

fix deprecations in example/tutorial usage [circle full]

53cb5cd

fix tutorial errors

35a425c

drammock added the EOSS4 label Sep 29, 2022

drammock added 5 commits September 30, 2022 10:17

fix show_names in plot_regression_weights

c8a8fe9

make title legible

4d3ae42

fix default n contours

dedb1d7

[unrelated] remove duplicate TOC from tutorial

afa43bf

restore explicit vlims

9011402

drammock enabled auto-merge (squash) September 30, 2022 15:51

drammock merged commit 45720cc into mne-tools:main Sep 30, 2022

drammock deleted the standardize-topomap-args branch November 16, 2022 14:47

drammock mentioned this pull request Dec 15, 2022

Fix lingering topomap api #11371

Merged

Uh oh!

Standardize topomap args #11123

Standardize topomap args #11123

Uh oh!

Conversation

drammock commented Aug 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmagnuski Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

drammock Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

drammock Sep 15, 2022

Choose a reason for hiding this comment

Uh oh!

mmagnuski Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

drammock commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agramfort commented Sep 10, 2022 via email

Uh oh!

larsoner commented Sep 28, 2022

Uh oh!

larsoner commented Sep 29, 2022

Uh oh!

larsoner commented Sep 29, 2022

Uh oh!

drammock commented Sep 30, 2022

Uh oh!

larsoner commented Sep 30, 2022

Uh oh!

agramfort commented Oct 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

drammock commented Aug 31, 2022 •

edited

Loading

drammock commented Sep 9, 2022 •

edited

Loading