-
Notifications
You must be signed in to change notification settings - Fork 21
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
[WIP] TimeFrequency CSP Class #28
base: main
Are you sure you want to change the base?
Conversation
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.
Some pitpicks on the code. But uhm, where is the actual CSP happening in this code?
from mne.filter import filter_data | ||
|
||
|
||
class TimeFrequencyCSP(object): |
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.
Since this class follows the scikit-learn API, can we make this a subclass of sklearn.base.BaseEstimator
and add the sklearn.base.TransformerMixin
?
Times in seconds. Upper bound of the time over which to estimate | ||
signals. | ||
freqs : array of floats | ||
Frequency values over which to compute time-frequency decomposition. |
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.
Can you elaborate a bit on how exactly to pass the frequency ranges? Maybe give an example here?
from sklearn import clone | ||
|
||
# Crop data into time-window of interest | ||
Xt = epochs.copy().crop(w_tmin, w_tmax).get_data() |
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.
Why Xt
and not just X
? I read it first as meaning X.T
, but below, it is passed directly to the sklearn estimators...
self._window_spacing)[1:] # noqa | ||
self._n_windows = len(self._centered_w_times) | ||
|
||
def _transform(self, epochs, y, w_tmin, w_tmax, method='', pos=[]): |
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.
Why do the method
and pos
params have default values? The defaults don't make much sense to me.
# filter the data at the desired frequency | ||
epochs_bp = epochs.copy() | ||
epochs_bp._data = filter_data(epochs_bp.get_data(), self.sfreq, | ||
fmin, fmax, verbose='CRITICAL') |
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.
A thought: maybe it is helpful to expose some parameters for the filtering? Perhaps not, as the number of parameters to this class will grow quite large.
Developing a Class from the time-frequency CSP decoding example
(https://4177-1301584-gh.circle-artifacts.com/0/home/ubuntu/mne-python/doc/_build/html/auto_examples/decoding/plot_decoding_csp_timefreq.html#sphx-glr-auto-examples-decoding-plot-decoding-csp-timefreq-py)
cc @kingjr