-
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
Improvements to the ICA API #10601
Comments
I like it a lot! |
I'm fine with moving this functionality to a unified |
Yes please no API changes here. Moving the code outside of MNE is a great idea though. |
let's not rush here. Let mne-icalabel get a bit of visibility and user
feedback and then let's
reevaluate in a few months.
… Message ID: ***@***.***>
|
Does MNE-ICLabel require PyTorch? If that's the case, given @hoechenberger's difficulties with installing PyTorch in mne-tools/mne-installers#123 as it's quite a big dependency, I'm -1 on depending on MNE-ICLabel for core ICA functionality in MNE-Python. We should only depend on it for ICLabel functionality. In other words, let's add |
|
My proposal above is actually not to do this, but to have as much I am thinking that if a user uses the new API to do Then if a user does |
Oh, yes we can do it this way as well. I was proposing to move all IC-labelling stuff to So compare to your proposal, the idea is to import |
I like this proposal a bit less because if a user does |
So it sounds like the only change we should do is #9846? Is there a consensus there? What are the next steps to enable storage of "scores" within the ICA labels? |
And add the |
It would, but then we'd have to add another hard dependency. Dependencies aside, though, I like the idea of keeping mne-iclabel's functionality focused on iclabel stuff as opposed to things like |
So, what is 'iclabel-specific'? We also want to include different models to label components, not just But anyway, that was just an additional proposal to move some of the code from |
Let's do it the other way around, then: The package depends on MNE already, so all would be good, I suppose! |
To me the default
This could work, too, I suppose. But it sounds like you're saying that we shouldn't add |
Installing pytorch with installer seems ambitious unless you accept to only
install a CPU version.
but then you'll have people complaining to have a slow pytorch if they do
anything else.
I don't know if the installer can detect at runtime if cuda is available
and if so install a GPU version.
It seems a bit of work...
… Message ID: ***@***.***>
|
yes for icalabel cpu is just what you need but if folks want to train deep
net in the MNE environment
it would be bad. Not sure what to recommend.
… Message ID: ***@***.***>
|
yes.. thought about it after, you are right. |
If we include ICALabel, I wouldn't mention the availability of PyTorch as long as we only have CPU support. Capability detection & package selection at installation time is possible (CPU vs CUDA); the auto-selection is already part of the |
@adam2392 pinging as discussed.
Hello! Following the release of
mne-icalabel
, I'd like to propose a couple of changes to theICA
API.In
mne-icalabel
, we usedlabel_components
as the main entry-point:The name was chosen to be consistent with the methods in an
ICA
instance:get_components
andplot_components
. For now, the only available method argument is'iclabel'
, but the goal is to add more.-> Proposed change: add a
label_components
method to theICA
instance that takes as inputself
, a raw or epochs instance, and the name of the method to use to label components.But instead of only stopping here and clogging a bit more the MNE API, I'd like to go a bit further. For now, the ICA instance has build-in simple methods to label cardiac, ocular, MEG ref, and muscular-related components:
find_bads_eog
,find_bads_ecg
,find_bads_ref
and the newly addedfind_bads_muscle
. Each of those methods' outputs is very similar to the output oflabel_components
: labels and scores/prediction probabilities.-> Proposed change: move those simple labeling methods to
mne-icalabel
, deprecate thefind_bads_
methods, and group all labeling methods under a simpleica.label_components
method.IMO, this change would simplify the API and move the maintenance of the related code and documentation to the
mne-icalabel
repository.And finally, alongside those 2 changes, labels and scores/prediction probabilities could be stored in the ICA instance. For now, only the labels are stored in a dictionary
self.labels_
where the key is the method/component type and the value is a list of components IDx that were labelled with this type. Related issue: #9846WDYT?
The text was updated successfully, but these errors were encountered: