-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add spectrum class #10184
add spectrum class #10184
Conversation
6f28417
to
08b044f
Compare
this is still a draft but now would be a good time for some early feedback about code organization and API (ping @larsoner and @agramfort in particular, but input is welcome from anyone). |
I am wondering about the use of spectrum. We use psd everywhere else. my reflex would have been psd = raw.compute_psd() would be the same as what we have now raw.plot_psd() |
I chose "spectrum" because it's more general: e.g., depending on what parameters people use, sometimes we plot the amplitude spectrum, or we plot 10*log10 of the power spectrum. |
Other reasons to pick a new name:
|
ok makes sense
… Message ID: ***@***.***>
|
7352e35
to
6d36f1d
Compare
3be6bcc
to
0042f0a
Compare
@dengemann tagging myself to follow. Great this is alive! |
Really happy to see this @drammock. Once basics are settled it would be cool to also add |
6a3335c
to
88a7ef9
Compare
b92da83
to
31bc67a
Compare
f971b1f
to
6282339
Compare
5f1e333
to
fb200bc
Compare
@requires_h5py | ||
@pytest.mark.parametrize('inst', ('raw', 'epochs', 'evoked')) | ||
def test_spectrum_io(inst, tmp_path, request): | ||
"""Test save/load of spectrum objects.""" | ||
inst = request.getfixturevalue(inst) | ||
fname = tmp_path / 'spectrum.h5' | ||
orig = inst.compute_psd() | ||
orig.save(fname) | ||
loaded = read_spectrum(fname) | ||
assert orig == loaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@larsoner this test fails for evoked because of a limitation in pytest. The failure is:
_____________________________________________ test_spectrum_io[evoked] ______________________________________________
The requested fixture has no parameter defined for test:
mne/time_frequency/tests/test_spectrum.py::test_spectrum_io[evoked]
Requested fixture '_evoked' defined in:
mne/conftest.py:306
Requested here:
/opt/miniforge3/envs/mnedev/lib/python3.10/site-packages/_pytest/fixtures.py:659
I can make it go away by removing params=[testing._pytest_param()]
from the signature here:
Lines 305 to 312 in 2ba483a
@pytest.fixture(scope='session', params=[testing._pytest_param()]) | |
def _evoked(): | |
# This one is session scoped, so be sure not to modify it (use evoked | |
# instead) | |
evoked = mne.read_evokeds(fname_evoked, condition='Left Auditory', | |
baseline=(None, 0)) | |
evoked.crop(0, 0.2) | |
return evoked |
But presumably that will break your magic auto-skipping behavior. The limitation is that request.getfixturevalue()
cannot handle fixtures that have params, and this is apparently by design. A workaround is to use the pytest-lazy-fixture package. Another alternative is to copy-paste a separate test for Evoked IO, like this:
@requires_h5py
def test_evoked_spectrum_io(evoked, tmp_path):
"""Test save/load of spectrum objects."""
fname = tmp_path / 'spectrum.h5'
orig = evoked.compute_psd()
orig.save(fname)
loaded = read_spectrum(fname)
assert orig == loaded
... which works because now the evoked
fixture is listed directly in the test signature (instead of being accessed via getfixturevalue()
), but is not very DRY.
What do you recommend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would work around it with:
@requires_h5py
@pytest.mark.parametrize('inst', ('raw', 'epochs', 'evoked'))
def test_spectrum_io(inst, tmp_path, request, evoked):
"""Test save/load of spectrum objects."""
# work around parametrized fixtures not being accessible
# via request.getfixturevalue
# https://github.com/pytest-dev/pytest/issues/4666#issuecomment-456593913 # noqa: E501
if inst == 'evoked':
inst = evoked
else:
inst = request.getfixturevalue(inst)
fname = tmp_path / 'spectrum.h5'
orig = inst.compute_psd()
orig.save(fname)
loaded = read_spectrum(fname)
assert orig == loaded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, OK. Inelegant but practical. Will do.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a work of art! 🤩
Do we at some point want to support users creating their own Spectral
instances like EpochsArray
and EvokedArray
?
this is something I struggled with, and ultimately decided "YAGNI". But, I suspect it wouldn't be too difficult given that it's already basically implemented in the |
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
Fair enough. |
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1 for merge after the _ci
fix!
Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com>
Congrats @drammock ! |
Nice one! |
🎉 🍻 👏 @drammock |
* STY: alphabetize imports * wip: first sketch of spectrum class [ci skip] implement __repr__; add placeholder _repr_html_ add draft of _repr_html_ default to multitaper for Evokeds make raw.plot_psd() use the new code path unify viz.plot_raw_psd code path too support unaggregated multitaper add picks param to spectrum.plot() fix(ish) the units() method allow average=False as synonym for None handle unaggregated estimates in combo with epochs fix CI plotting implement get_data method [ci skip] * refactor aggregation * fix instance type checking * improve TODO notes * WIP use new class in plot_psd_topo [ci skip] * docdict additions * implement Spectrum.to_data_frame * test Spectrum.to_data_frame * adapt to_data_frame for unaggregated spectra [ci skip] * test unaggregated welch to df * test unagg multitaper to df * make tests more similar * make DRY * fix epoch test * simplify test * fix flake * use requires_pandas * fix bad rebase * fix API for epochs * use new API in example * tiny docstring improvement * fix unused import * fix docdict key order * do it the smarter/safer way * update tests to avoid deprecated calls * better deprectation message * convert more deprecated func calls to new method * unused imports * fix tests * more unused imports * fix circular imports also: - apply isort to a couple files - revert distracting isort on otherwise barely touched file * make CIs pass * get I/O working * don't store verbose attr * test IO * fix D202 * fix flake * better docstring for save method and read func * fix compute_psd docstring * return value descr * decorate test (h5py) * fixup after rebase * reorder methods * add __getitem__ functionality * __getitem__ tests * add __eq__, test for .copy(), refactor IO test * refactor to separate epochs class * EpochsSpectrum IO * fix type checking, better variable naming * test evoked IO too [ci skip] * fix type checking some more * adjust .units() for complex multitaper data * fix flake * docstring refactor * working plot_topomap implementation * fix docstring tests * fix pydocstyle * add EpochsSpectrum to the public API * make test more DRY * tweak deprecation message * docdict/docstrings fixes * add plot_psd_topomap to mixin * pytest limitation workaround * work toward unifying plot_topomap API * don't silently overwrite units * TODO comments [ci skip] * WIP tutorial changes * TODO: plot_psd_topo * WIP plot_topo & docdict stuff * plot_topomap docstring dedup * dedup legacy n_fft default * fix varname * more WIP plot_psd_topo * finish migrating plot_psd_topo to mixin * use new code path for plot_topomap * flake * docstring tests * unused imports * whitespace * flake * flake again * add plot_topo as spectrum method * don't do too much * fix test * flake * fix test * fix tutorial * WIP spectrum class tutorial * codespell * update tutorial and repr_html template * better repr, better shape checking * tweaks from self-review * update changelog * flake * fix * undo isort / other unrelated changes * use new API in tutorial * remove redundant plt_show * standardize docstring order * explain setup.cfg entry * fix html repr of units * simplify __eq__ by improving object_diff * remove deepcopy override * fix flake8 config * remove superfluous BibTeX fields [ci skip] Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com> * misc fixes [ci skip] Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com> * Update mne/viz/utils.py [ci skip] Co-authored-by: Eric Larson <larson.eric.d@gmail.com> * update old tutorials more thoroughly * file encoding / test comments Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com> * use __setstate__ and __getstate__ * flake * fix reject_by_annot appearing where it shouldn't * fix docstrings Co-authored-by: Marijn van Vliet <w.m.vanvliet@gmail.com> Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
* upstream/main: Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104) MRG: Fixes for mne-tools#11090 (mne-tools#11108) add test for edf units param (mne-tools#11105) BUG: Improve logic for bti (mne-tools#11102) add spectrum class (mne-tools#10184) ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099) ENH: Add temperature and galvanic (mne-tools#11090)
* upstream/main: (366 commits) BUG: Spectrum deprecation cleanup [circle deploy] (mne-tools#11115) Add API entry list and map (mne-tools#10999) Add legacy decorator (mne-tools#11097) [ENH, MRG] Add time-frequency epoch source estimation (mne-tools#11095) Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104) MRG: Fixes for mne-tools#11090 (mne-tools#11108) add test for edf units param (mne-tools#11105) BUG: Improve logic for bti (mne-tools#11102) add spectrum class (mne-tools#10184) ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099) ENH: Add temperature and galvanic (mne-tools#11090) Add error message when conversion of EEG locs to head space fails (mne-tools#11080) DOC: removed unnecessary line in PSF example (mne-tools#11085) FIX: Update helmet during ICP (mne-tools#11084) Fix various typos (mne-tools#11086) DOC: Rel BUG: don't assume that channel info contains particular keys (mne-tools#11074) [BUG] Fix plot_topomap with sphere="eeglab" (mne-tools#11081) Add `vmin` and `vmax` specification to `mne.Evoked.animate_topomap` (mne-tools#11073) Add _on_missing functionality to UpdateChannelsMixin (mne-tools#11077) ...
* upstream/main: (25 commits) DOC: Exclude some implicit refs in doc build (mne-tools#10433) MAINT: Better issue forms (mne-tools#11101) [FIX] Typo in example (mne-tools#11118) [MRG] Improve interpolation of bridged electrodes (mne-tools#11094) BUG: Spectrum deprecation cleanup [circle deploy] (mne-tools#11115) Add API entry list and map (mne-tools#10999) Add legacy decorator (mne-tools#11097) [ENH, MRG] Add time-frequency epoch source estimation (mne-tools#11095) Revert "Add error message when conversion of EEG locs to [circle deploy] (mne-tools#11104) MRG: Fixes for mne-tools#11090 (mne-tools#11108) add test for edf units param (mne-tools#11105) BUG: Improve logic for bti (mne-tools#11102) add spectrum class (mne-tools#10184) ENH : add units parameter to read_raw_edf in case units is missing from the file (mne-tools#11099) ENH: Add temperature and galvanic (mne-tools#11090) Add error message when conversion of EEG locs to head space fails (mne-tools#11080) DOC: removed unnecessary line in PSF example (mne-tools#11085) FIX: Update helmet during ICP (mne-tools#11084) Fix various typos (mne-tools#11086) DOC: Rel ...
Progress so far:
.compute_psd()
method for Raw, Epochs, Evoked yields Spectrum objectaverage=False
output='complex'
support passing a callable to aggregate welch/multitaper estimatessupport passing a callable to aggregate welch/multitaper estimates in Spectrum constructor #11093._dims
, e.g.,('epoch', 'channel', 'freq', 'taper')
for an unaggregated multitaper derived from Epochsspectrum.save(fname)
/mne.time_frequency.read_spectrum(fname)
) for Spectrum object__eq__
method for comparisons / testing__getitem__
works for raw/epochs/evoked (raw/evoked-derived spectra work like np.array; epochs-derived spectra work like epochs: select by condition name, pandas query, etc).pick()
method.units()
method.to_data_frame()
method (works with raw/epochs/evoked, agged/unagged estimates).plot()
method with equivalent API to oldinst.plot_psd()
method.plot_topomap()
method.plot_topo()
methodyields this:
spectrum
has properties_data
,freqs
,ch_names
,method
(welch or multitaper), aninfo
dict, andspec._dims
(description of the data dims). For exampleHere is the corresponding repr
<Spectrum (from Raw) | 364 channels × 129 freqs × 651 segments, 0.0-300.3 Hz>
Eventually (in another PR) I'd like to refactor the multitaper functions to make it possible to return the unaggregated tapersEDIT I think this was done already while I was on leave, in which case you'd expect something likeraw.to_spectrum(multitaper, aggregate=False)._dims
to be('channel', 'freq', 'taper')
.epochs.compute_psd()
andevoked.compute_psd()
also work: