Extend plot_csd support to SEEG, ECoG, and DBS channel types#13713
Extend plot_csd support to SEEG, ECoG, and DBS channel types#13713Aniketsy wants to merge 11 commits intomne-tools:mainfrom
Conversation
tsbinns
left a comment
tsbinns
left a comment
There was a problem hiding this comment.
Thanks for opening this, definitely on the right track! Some comments below.
|
@tsbinns thanks for the review and for suggesting this approach, it makes much more sense than my earlier implementation. please have a look and let me know if any further improvements are needed. |
tsbinns
left a comment
tsbinns
left a comment
There was a problem hiding this comment.
Good changes! A couple small comments on the code, and a slightly bigger one for to refine the test. Let me know if anything's unclear.
|
@larsoner @wmvanvliet I noticed that the existing behaviour when plotting is that if no valid channel types are present (currently only EEG, Mag, Grad; this PR would change it to all valid data types), it just returns an empty I was a bit surprised by this. My expectation was at least a warning would be given that there is nothing valid to plot. WDYT? |
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
tsbinns
left a comment
tsbinns
left a comment
There was a problem hiding this comment.
Good progress! A couple minor comments to tidy things up.
mne/viz/misc.py
Outdated
| # The units in which to plot the CSD | ||
| units = dict(eeg="µV²", grad="fT²/cm²", mag="fT²") | ||
| scalings = dict(eeg=1e12, grad=1e26, mag=1e30) | ||
| indices, titles, _, _, ch_types = _index_info_by_ch_type(info, csd.ch_names) |
There was a problem hiding this comment.
Why not use the units and scalings returned here rather than re-finding them below?
There was a problem hiding this comment.
yes , that make sense this will reduce redundancy
mne/viz/misc.py
Outdated
| if mode == "csd": | ||
| scaling = DEFAULTS["scalings"].get(ch_type, 1) | ||
|
|
There was a problem hiding this comment.
Can be removed if using the scalings returned above.
mne/viz/misc.py
Outdated
| """Build channel-type-grouped indices and metadata for an object's channels. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| info : Info | ||
| The measurement info, already restricted to the channels of interest | ||
| (e.g. via :func:`pick_info`). | ||
| obj_ch_names : list of str | ||
| Channel names of the object being indexed (e.g. the channel list of a | ||
| :class:`~mne.time_frequency.CrossSpectralDensity` or the filtered | ||
| channel list derived from a :class:`~mne.Covariance`). | ||
|
|
||
| Returns | ||
| ------- | ||
| indices : list of list of int | ||
| Channel indices into *obj_ch_names*, one list per channel type. | ||
| titles : list of str | ||
| Human-readable title for each channel type. | ||
| units : list of str | ||
| Unit string for each channel type. | ||
| scalings : list of float | ||
| Scaling factor for each channel type. | ||
| ch_types : list of str | ||
| Channel type key for each group. | ||
| """ |
There was a problem hiding this comment.
We often forgo detailed docstrings for small private functions where the inputs/outputs are clear. In this case info and ch_names are ubiquitous and don't need explanation, and now the outputs are named, so I'd say this can be removed (just keep the first line).
There was a problem hiding this comment.
got it that make sense
mne/viz/misc.py
Outdated
| ) | ||
|
|
||
|
|
||
| def _index_info_by_ch_type(info, obj_ch_names): |
There was a problem hiding this comment.
Just ch_names would be the typical param name.
mne/viz/tests/test_misc.py
Outdated
| @pytest.mark.parametrize( | ||
| "ch_types, expected_n_figs", | ||
| [ | ||
| (None, 1), | ||
| ("eeg", 1), | ||
| ("ecog", 1), | ||
| (["grad", "dbs"], 2), | ||
| ("misc", 0), | ||
| ], | ||
| ) | ||
| def test_plot_csd(ch_types, expected_n_figs): |
There was a problem hiding this comment.
I would prefer if expected_n_figs is set programatically within the test function, i.e., based on the number of valid data types in ch_types (with an exception for when ch_types is None).
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
Co-authored-by: Thomas S. Binns <t.s.binns@outlook.com>
2 participants
Fixes #13711