-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
FIX: 0-index ICA channel names in get_sources #3633
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
Conversation
Current coverage is 74.13% (diff: 0.00%)@@ master #3633 diff @@
==========================================
Files 343 343
Lines 60888 60761 -127
Methods 0 0
Messages 0 0
Branches 9307 9301 -6
==========================================
- Hits 53220 45046 -8174
- Misses 4912 13434 +8522
+ Partials 2756 2281 -475
|
mne/preprocessing/ica.py
Outdated
| ch_info = info['chs'] = [] | ||
| for ii in range(self.n_components_): | ||
| this_source = 'ICA %03d' % (ii + 1) | ||
| this_source = 'ICA %03d' % (ii) |
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.
channel names for ICA are called ICA 001, ICA 002 etc.
if you want to display as number it should be displayed as ICA #0, ICA #1 etc.
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.
I've updated it so that component name is 'IC #000'
It makes sense to me that both plot_components and get_sources should have the same indexing. Otherwise it can be hard to keep track if you are comparing across plots. Other channel types seem to be 1-indexed though (at least for KIT) so I see the point.
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.
I also like IC #000, but you should make sure that the naming is consistent across mne.
BTW @agramfort managed to reference first PR to mne python :)
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.
@mmagnuski You mean that we should ensure consistency across mne with regards to IC plots, yes? Not that we should 0-index the naming for all channel types. I am a fan of 0-indexing everything but I guess other channel names are one-based indexed for a reason.
A great accidental reference from @agramfort
|
we discussed this many times conclusion so far is:
|
|
Okay fair enough. Thanks for clearing it up. Actually, as I was trying to figure out evoked.plot functionality regarding this I couldn't actually get it to plot without error (before all this, I was just extracting the data and using plt.plot()). The error I found is described in Issue #3647 Perhaps, after that is sorted out, we can come back to this. I'm wondering what you think about creating an option for the user to change format of plot legends. Perhaps (probably) that is already implemented and I don't know about it. |
|
closing? |
Tiny fix to make ICA channel names 0-indexed in the output of ica.get_sources as they are in the ICA plot functions.
@dengemann