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
ENH: decoding API #2290
Comments
Yes, that could be useful to have : |
I was just looking into TFR decompositions in MNE - sorry if I am late to the party here, but I'm wondering why functions like PSDEstimator exist in a different place than the time_frequency module. Does it do anything different from the functions in time_frequency (beyond being a class instead of function)? |
I think the idea was to be able to use these objects in an sklearn pipeline. If they don't follow sklearn API, you can't do that. |
yeah, that makes sense to me. I could just see people getting confused as to why there's this other function here that seems replicated in the time_frequency section. Probably not a big deal but it took me a few passes through the code to realize it was just a convenient wrapper to structure stuff w/ sklearn |
and I should mention - this does also implement a helpful new addition, which is running a multitaper PSD on epochs objects. Right now multitaper_psd expects |
Ah, I think this would be a good candidate for improved documentation. Relates to #1495. If you feel like making a contribution, it would be very welcome and much appreciated. Maybe, it should go into a new section on |
Sounds good - think it's better to include in a docstring of one of these functions, or as a separate tutorial kind of thing? It would be useful if there was a single notebook on "estimating the PSD with MNE-python" |
I'm +1 on this. |
On that note - would people be +1 to this function adding a 'freqs' attribute when it estimates the PSD, rather than just throwing it away? And potentially moreover, adding a 'data' attribute once it's run as well, so that it can be contained within the object? I see that this is starting to stray from the original intended usage, but I think this is a useful structure for estimating the PSD in general. |
See #2710 for a first pass |
You might want to update the "See Also" section of the docstring. I'm not super sure of retaining attributes. They are usually retained in the mne-python/mne/time_frequency/psd.py Line 116 in ec092ad
+1 on a simple tutorial. |
There is a similar function for epochs, but it hasn't been updated to include the multitaper PSD. That's why I assumed that this class was actually an extension to the methods in time_frequency |
umm ... no, I think this class is meant for a different purpose -- for decoding. I guess you could use this for now if it does your job. But the cleaner approach would be to update the function for epochs. @agramfort might be able to advice better. |
Hey @choldgraf : were you looking for this? http://martinos.org/mne/stable/generated/mne.time_frequency.tfr_multitaper.html?highlight=tfr_multitaper#mne.time_frequency.tfr_multitaper update: I actually realized that this is for TFR ... so yeah, we are still missing something for PSD |
I think the confusion comes from the fact that this class adds something different that doesn't exist in time_frequency. To my knowledge, in time_frequency you have a few options:
So this is the only thing that both allows you to use an epochs-shaped array and lets you use a multitaper method for calculating a PSD I agree that the decoding stuff should be in |
yeah, I agree that it's confusing. If you can help make it consistent and/or improve the documentation via manual / tutorial pages, that's more than welcome :) |
Well I guess the question is do we: A. Make it clearer with documentation etc, or I'm partial to B... |
yeah, I'm good with B. +1 from my end. Any other opinion? |
Maybe @trachelr could comment on this ... |
Cool - I can make a PR out of it if the addition isn't too complex. I'll wait to see if others have thoughts. |
the classes in decoding are meant to be used for sklearn pipelines. They
were created before the tfr_XXX functions. I would recommend to stick to
and improve what is time_frequency module and keep the decoding / ML use
case in it's world.
|
+1 for option B and make tutorials for decoding |
we've made progress now. Closing |
I suggest to enhance the decoding API. Actually there's a classifier.py module in which there is a set of transformers (Scaler, ConcatenateChannels, FilterEstimator, PSDEstimator).
I think we should rename this module to a transformer.py and create 2 modules, one for classification and another for regression, in which classes like LinearClassifier / LinearRegressor would be included.
The idea is to have a bench of transformers that could be chained into a sklearn pipeline with a LinearClassifier (or Regressor). It would look like this :
The LinearClassifier would be a simple wrapper of sklearn.linear_model which benefit for the compute_pattern function.
The idea is to have a mne.decoding tools (transformers, classifiers, regressors) that could be easily chained into a pipeline while keeping info throughout the pipeline.
What do you this about this?
The text was updated successfully, but these errors were encountered: