-
Notifications
You must be signed in to change notification settings - Fork 15
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
Discussion about the API for mne_icalabel and mne #19
Comments
This is essentially what I had in mind, so very much agreed.
Yes I believe this is a good route as well, so that way we only keep a maintained API of what is necessary from a user/researcher perspective.
I believe these methods are all using some pattern matching, so yes I agree, we can make a PR in MNE-Python when this package is ported.
I've made a change to FIFF before, so I think this will be pretty straightforward.
Agreed. Let's raise this point to MNE developers ^ I think to achieve all the above, after #18 is merged, we will need to do the following:
Then we can begin planning the next phase of improvements. I think the MNE community will be excited at this new feature. |
I'm glad we had the same idea in mind. Those steps work for me and make perfect sense! 🔥 |
So I think in line with something like sklearn, we have the following semantics:
This would need to be a class though. I can add this in #31 Besides this, I think a Moreover, I think the only necessary input to any function is WDYT? |
For the For running the ICA in the background, sure, we can run it by default with sensible parameters if it is not provided, or we can fit it if it's provided but I am a bit worried the transformer is adding unnecessary complexity. As you say, the I am also a bit worried that the winner-takes-all strategy is a bit risky. Using it by default to determine the class of each component is fine.. as long as the components who lost are not automatically added to I think it would be good to store the result of |
Ah I see, so the proposal here is to migrate this altogether into MNE, so that way it's structured within the I'm in favor of then making the output here in this package a dictionary as you said:
The user can then pick the one they want. This can also be easily transformed to a data frame in one line. I guess prolly better not to return a data frame, so we don't introduce a hard-dependency on pandas. Once this is done, sounds like the bulk of work requires going to MNE-Python. |
Just notes for the steps to integrate the underlying data structures for the
Check mne.constants.io.py look if any constants are close enough for the additional variables we want to save. For example, ICA annotation author via the existing constant |
I would like to discuss a bit what the API would look like, both on the
mne_icalabel
side and on themne
side. Here is my point of view:mne_icalabel
As described in the readme, this package is not only a port of the popular
ICLabel
network for EEG but could host other models for EEG, MEG, and iEEG. Thus, I would structure the package as:The main entry point from a user perspective would be in
mne_icalabel/icalabel.py
through a function that takes as input araw
orepochs
instance, an ICA decomposition, and a method (str). Themethod
argument would have a couple of accepted values: the name of the models, e.g.'iclabel'
; and would be responsible for selecting the model used.Based on the model used, the number of class in which the components are classified may vary. But in any case, the output should be the label (which class) and the probability that each component is in each class (scores).
Additionally to this main entry point, I would keep public a function to retrieve the features of a model, and a function to run a set of features through the model. e.g. for ICLabel, it would be
mne_icalabel/iclabel/features.py:get_features
andmne_icalabel/iclabel/network.py:run_iclabel
. Everything else would become private.mne
In version 1.1, there are already 4 methods to find bad components in
mne.preprocessing.ICA
:find_bads_ecg
,find_bads_eog
,find_bads_muscle
andfind_bads_ref
. Following this pattern, I would go for afind_bads
method that would wrap around the entry point inmne_icalabel/icalabel.py
, with the 2 parametersinst
andmethod
. As for the 4 other methods, the label can be stored inica.labels_
.I think it would be worth also storing the probability that each component is in each class (scores) in a similar attribute to
ica.labels_
, for both this newfind_bads
method and for all others as well.I brought the idea here a while ago: mne-tools/mne-python#9846 but I didn't follow up on it yet. It does require a new ICA FIFF field for a 2D matrix of floats.
Finally, from an MNE perspective, the API could be simplified a bit further. At term, the
find_bads
method could be the only one remaining with the 4 others moved tomne_icalabel
as models and called throughfind_bads
with the correct method keyword specified.The text was updated successfully, but these errors were encountered: