-
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
[MRG] ENH/DOC: demo custom spectrum creation #11493
Conversation
Co-authored-by: Richard Höchenberger <[email protected]>
Co-authored-by: Richard Höchenberger <[email protected]>
dims=('channel', 'freq'), | ||
freqs=freqs, | ||
inst_type_str='Raw', | ||
data_type='simulated signals', |
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.
@drammock I put this for illustration purposes. Are there any expectations in the Spectrum code regarding this field? It was 'Averaged EEG' in my concrete case on which this example is based.
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.
right now it is only used in the repr
of the object. I don't have any concrete plans to use it in any other way (and I would actively avoid using it in if
clauses to triage different flavors of spectrum, it's probably not reliable to do so)
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.
native values for this are Fourier Coefficients
, Power Spectrum
, and Averaged Power Spectrum
. Averaged Fourier Coefficients
is not supported, see
mne-python/mne/time_frequency/spectrum.py
Lines 1109 to 1118 in 18474b1
# averaging unaggregated spectral estimates are not supported | |
if hasattr(self, '_mt_weights'): | |
raise NotImplementedError( | |
'Averaging complex spectra is not supported. Consider ' | |
'averaging the signals before computing the complex spectrum.') | |
elif 'segment' in self._dims: | |
raise NotImplementedError( | |
'Averaging individual Welch segments across epochs is not ' | |
'supported. Consider averaging the signals before computing ' | |
'the Welch spectrum estimates.') |
CircleCI failure is this:
it's just a warning, but it's annoying when this breaks our CIs. Makes me wonder if we should either (1) make it a |
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.
changes LGTM. +1 for merge when CIs are green.
My one hesitation is about data_type
: I might be more comfortable sticking with Power Spectrum
here, since our tutorials are implicitly supposed to demonstrate best practices and Power Spectrum
would be the most consistent description of what the data are.
Comments addressed, CIs green. Good to go from my side. |
The only thing I'd change is the indentation of the custom function header; I would blackify it |
Co-authored-by: Richard Höchenberger <[email protected]>
* upstream/main: (46 commits) Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499) Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473) MAINT: Fix Circle [circle deploy] (mne-tools#11497) MAINT: Use mamba in CIs (mne-tools#11471) Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479) Fix typo in tutorial (mne-tools#11498) Typo fix and added colons before code (mne-tools#11496) [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493) Accept only left-clicks for adding annotations (mne-tools#11491) [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489) [ENH] Added unit_role to add non-breaking space between magnitude and units (mne-tools#11469) MAINT: Fix CircleCI build (mne-tools#11488) [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475) Typo fix (mne-tools#11485) [MRG] Forward argument axes from plot_sensors to DigMontage.plot (mne-tools#11470) [MRG] Improve error message raised on channels missing from DigMontage (mne-tools#11472) MAINT: Deal with pkg_resources usage bugs (mne-tools#11478) Add object array support and docstring (mne-tools#11465) [ENH] Adjusted SSD algorithm to support non-full rank data (mne-tools#11458) [BUG] fix nibabel reference (mne-tools#11467) ...
* upstream/main: (264 commits) BUG: Fix deprecated API usage in example (mne-tools#11512) Deprecate 'kind' and 'path' in favor of 'fname' in the layout reader (mne-tools#11500) EGI/MFF events outside EEG recording should not break the code (mne-tools#11505) fixed annotations error on export (mne-tools#11435) DOC: Update installer links [skip azp] [skip actions] [skip cirrus] (mne-tools#11506) BUG: updates for MPL 3.7 compatibility (mne-tools#11409) Fix docstrings by replacing str with path-like and fix double backticks for formatting (mne-tools#11499) Use pathlib.Path instead of os.path to handle files and folders [circle deploy] (mne-tools#11473) MAINT: Fix Circle [circle deploy] (mne-tools#11497) MAINT: Use mamba in CIs (mne-tools#11471) Updating documentation to clarify full vs half-bandwidth and defaults in time_frequency.multitaper.py (mne-tools#11479) Fix typo in tutorial (mne-tools#11498) Typo fix and added colons before code (mne-tools#11496) [MRG] ENH/DOC: demo custom spectrum creation (mne-tools#11493) Accept only left-clicks for adding annotations (mne-tools#11491) [BUG, MRG] Fix pial surface loading, logging in locate_ieeg (mne-tools#11489) [ENH] Added unit_role to add non-breaking space between magnitude and units (mne-tools#11469) MAINT: Fix CircleCI build (mne-tools#11488) [DOC] Updated decoding.SSD documentation and internal variable naming (mne-tools#11475) Typo fix (mne-tools#11485) ...
What does this implement/fix?
Explain your changes.
A simple example of how to put custom power spectral data into an average Spectrum class.
A proper SpectrumArray class could follow if there is interest.
Additional information
Any additional information you think is important.
Discussed with @drammock @hoechenberger in past 2 office hours.