Adjust labelling to actual naming of ICs #4320

stfnrpplngr · 2017-06-15T12:10:37Z

I'm not sure whether this is a bug, just observed the following.
Plotting the time-locked sources doing an ICA, results in different labelling clicking on the components' lines:

Changing the proposed line in _export_info() solves the problem:

EMG channels were missing for the create_info function

Added emg channel type for use in the create_info function

During ICA, plotting of sources results in different labelling

agramfort · 2017-06-15T12:48:20Z

we keep getting these bug reports about channel names that don't correspond to indices.

when we write ICA #001 it corresponds to the indices but when the channel names named ICA001 start with 1 to match the convention of EEG channels that start with 1.

is this really worth changing?

codecov-io · 2017-06-15T13:05:08Z

Codecov Report

Merging #4320 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4320      +/-   ##
==========================================
+ Coverage   84.42%   84.42%   +<.01%     
==========================================
  Files         346      346              
  Lines       62897    62897              
  Branches     9672     9672              
==========================================
+ Hits        53099    53103       +4     
+ Misses       7118     7115       -3     
+ Partials     2680     2679       -1

larsoner · 2017-06-15T13:27:35Z

Given the persistent confusion I think we should change something. To me each new report adds to evidence that we have a bad design that needs fixing. We could try labeling as `ICA[3]` or so instead of `ICA #3`, but I still don't think it's very good. I'm starting to think that it would be better for ICA to number their channel names from zero. It creates an inconsistency with other channel types, yes, but I think it will create less confusion than the current implementation.

agramfort · 2017-06-15T13:38:46Z

agreed I give up :) let's call the ICA channels ICA 000, ICA 001, etc...

larsoner · 2017-06-15T13:49:12Z

Thanks for being the final straw @stfnrpplngr, closing this as dup of #3889 where we'll continue discussion

larsoner · 2017-06-15T14:08:22Z

Reopened because this is already along the lines of the right fix. @stfnrpplngr 3 more things are necessary to get this in:

Update ICA tutorials
Add the option to use old / bad naming style (like what's done in ICA get sources named w/ zero based indices #1622) w/deprecation cycle to move to the new / good naming style
Updating any tests

Do you have time to look into these? We're happy to walk you through them if you want to learn how.

stfnrpplngr · 2017-06-16T11:11:39Z

If someone could walk me through, that would be great.

larsoner · 2017-06-16T15:43:53Z

For (1), look through these to see if there is any mention of numbering differences:

Also look through #3633, #2019, #2000, #1622 to see if they mention updating docs anywhere else that we could fix.

For (2), you'd add a channel_start=None argument to ICA constructor and in __init__ do:

if channel_start is None:
    channel_start = 1
    warn('channel_start will default to 1 in 0.15, but change to 0 in 0.16. Set it explicitly to avoid this warning', DeprecationWarning)
if channel_start not in (0, 1):
    raise ValueError('channel_start must be 0 or 1, got %s' % (channel_start,))

For (3) run nosetests mne/preprocessing/tests/test_ica.py and ensure they pass. Run the two examples referenced above and compare the outputs to those from the current dev build. Push changes to your PR and see if the CIs (Travis, AppVeyor, and CircleCI) are happy.

larsoner · 2017-08-08T18:32:34Z

@stfnrpplngr do you have time to give it a try, or should someone else take over?

Adjusting the naming of sources to the naming in ica.plot_components()

Adding channel_start argument and deprecation warning Suggested changes regarding pull request

larsoner · 2017-08-09T14:14:05Z

tutorials/plot_artifacts_correction_ica.py

 raw = mne.io.read_raw_fif(raw_fname, preload=True)
-# 1Hz high pass is often helpful for fitting ICA
-raw.filter(1., 40., n_jobs=2, fir_design='firwin')
+raw.filter(1, 40, n_jobs=2)  # 1Hz high pass is often helpful for fitting ICA


Sorry, this probably happened updating from the master branch

…ch-3

stfnrpplngr · 2017-08-14T10:44:49Z

@larsoner would you mind having a look on it again?

dengemann · 2017-08-14T11:42:55Z

mne/preprocessing/ica.py

+                 DeprecationWarning)
+        if channel_start not in (0, 1):
+            raise ValueError('channel_start must be 0 or 1, got %s' %
+                             (channel_start,))


do we actually really need a parameter for this? Or is the plan to remove it after the deprecation cycle?

See suggestion above

#4320 (comment)

But maybe we don't need the cycle and can just make the API change

I'd opt for not deprecating. We could even twitter vote ... Let's just get it right once.

IIRC that simple change is what @stfnrpplngr had as the commit two months ago. :(

Sorry for the back and forth @stfnrpplngr, but can you change it just to use the index (no need for the param / deprecation I suggested before)? Other than that, the other thing I see is that doc/whats_new.rst needs to be updated in the API section saying that the ICA labeling now starts from 0.

dengemann · 2017-08-14T11:43:44Z

mne/preprocessing/ica.py

        ch_info = info['chs'] = []
        for ii in range(self.n_components_):
-            this_source = 'ICA %03d' % (ii + 1)
+            this_source = 'ICA %03d' % (ii)


we could really just make clear that these are the indices. We have a similari issue in #4049 and should solve it consistently.

yes maybe using the # sign

so "ICA #1" is clear that it's index 0?

I'd rather do [0] than #1. But earlier you seemed happy with the current implementation...

dengemann · 2017-08-14T14:46:38Z

My apologies @stfnrpplngr <https://github.com/stfnrpplngr> I did not follow closely enough so far. If you guys prefer a depracation I won't stop you. But don't do it for me.

…

On Mon, Aug 14, 2017 at 4:43 PM Eric Larson ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mne/preprocessing/ica.py <#4320 (comment)>: > @@ -224,7 +224,17 @@ class ICA(ContainsMixin): def __init__(self, n_components=None, max_pca_components=None, n_pca_components=None, noise_cov=None, random_state=None, method='fastica', fit_params=None, max_iter=200, - verbose=None): # noqa: D102 + verbose=None, channel_start=None): # noqa: D102 + + if channel_start is None: + channel_start = 1 + warn('channel_start will default to 1 in 0.15, but change ' + 'to 0 in 0.16. Set it explicitly to avoid this warning', + DeprecationWarning) + if channel_start not in (0, 1): + raise ValueError('channel_start must be 0 or 1, got %s' % + (channel_start,)) IIRC that simple change is what @stfnrpplngr <https://github.com/stfnrpplngr> had as the commit two months ago. :( Sorry for the back and forth @stfnrpplngr <https://github.com/stfnrpplngr>, but can you change it just to use the index (no need for the param / deprecation I suggested before)? Other than that, the other thing I see is that doc/whats_new.rst needs to be updated in the API section saying that the ICA labeling now starts from 0. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#4320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0firV-IeKxHtTEVK5G5wC9lnRXIT_xks5sYF0YgaJpZM4N7IFj> .

larsoner · 2017-08-14T14:51:06Z

I don't feel strongly either way, @stfnrpplngr feel free to revert to the simpler way or keep the current deprecation, up to you. Then we need doc/whats_new.rst update and we should be good

… in 0.16

stfnrpplngr · 2017-08-21T11:18:11Z

I would like to keep it containing the deprecation warning hopefully guaranteeing less confusion on the user site. The doc/whats_new.rst has been changed.

agramfort · 2017-08-21T12:19:10Z

so now the plan is to have ICA 000 as first ica channel? to everything is zero indexed?

larsoner · 2017-08-22T15:04:30Z

so now the plan is to have ICA 000 as first ica channel? to everything is zero indexed?

Yep

larsoner · 2017-08-22T15:09:56Z

mne/preprocessing/ica.py

                 n_pca_components=None, noise_cov=None, random_state=None,
                 method='fastica', fit_params=None, max_iter=200,
-                 verbose=None):  # noqa: D102
+                 verbose=None, channel_start=None):  # noqa: D102


add channel_start before verbose, and also document it

agramfort · 2017-08-22T15:09:58Z

ok let's see if it's actually less confusing for everyone...

agramfort · 2017-08-22T15:11:49Z

doc/whats_new.rst

 API
 ~~~
+
+	  - Add ``channel_start`` parameter in :func:`mne.preprocessing.ica` to make explicit that ICA component labelling starts from 1 in 0.15, default will change to 0 in 0.16 by `Stefan Repplinger`_ and `Eric Larson`_ 


over indented

agramfort · 2017-08-22T15:25:19Z

ok keep it simple stupid. Start naming at 0

larsoner · 2017-08-22T15:36:50Z

With no deprecation? That's okay with me I guess, and it sounded like @dengemann thought it was overkill, too.

@stfnrpplngr I'll close this, cherry-pick your commits (there are a couple that do what we want) and open a new PR. Thanks for the fix and iterations!

larsoner · 2017-08-22T15:50:27Z

yes maybe using the # sign

so "ICA #1" is clear that it's index 0?

BTW this suggestion is the opposite of how it works in ICA viz currently. "ICA #000" is index 0. Just more evidence that our current scheme is unintuitive.

stfnrpplngr added 6 commits March 13, 2017 16:42

Added EMG to MeasInfo

6ff21c5

EMG channels were missing for the create_info function

Update meas_info.py

23e252d

Added emg channel type for use in the create_info function

Adding 'emg' to create_info docstring

b05135a

Merge remote-tracking branch 'refs/remotes/mne-tools/master'

54fb02e

Merge remote-tracking branch 'refs/remotes/origin/patch-1'

9e59816

Adjust component labelling to component naming

ff2e4cd

During ICA, plotting of sources results in different labelling

larsoner closed this Jun 15, 2017

larsoner mentioned this pull request Jun 15, 2017

ica.get_sources has strange ch_names and ch_type #3889

Closed

larsoner reopened this Jun 15, 2017

larsoner added this to the 0.15 milestone Jun 15, 2017

Merge branch 'master' of https://github.com/stfnrpplngr/mne-python

b87bb57

stfnrpplngr added 4 commits August 9, 2017 13:10

Suggested changes regarding pull request

9756ecf

Adjust labelling to actual naming of ICs

a29f699

Adjusting the naming of sources to the naming in ica.plot_components()

Minor typos

d96e0fa

Adding channel_start argument to ica.py

c25547f

Adding channel_start argument and deprecation warning Suggested changes regarding pull request

stfnrpplngr force-pushed the patch-3 branch from a3a4e0f to c25547f Compare August 9, 2017 12:44

Merge branch 'master' into patch-3

b908e37

larsoner reviewed Aug 9, 2017

View reviewed changes

stfnrpplngr added 2 commits August 9, 2017 21:45

Reverting change in line 33

7441e38

Sorry, this probably happened updating from the master branch

Merge remote-tracking branch 'refs/remotes/mne-tools/master' into pat…

c48558c

…ch-3

dengemann reviewed Aug 14, 2017

View reviewed changes

stfnrpplngr added 2 commits August 21, 2017 13:11

Changelog in doc/whats_new.rst updated referring to new labelling…

82ca894

… in 0.16

Merge branch 'master' into patch-3

f811bc7

larsoner reviewed Aug 22, 2017

View reviewed changes

agramfort reviewed Aug 22, 2017

View reviewed changes

larsoner closed this Aug 22, 2017

stfnrpplngr deleted the patch-3 branch December 1, 2017 14:48

Uh oh!

Adjust labelling to actual naming of ICs #4320

Adjust labelling to actual naming of ICs #4320

Uh oh!

Conversation

stfnrpplngr commented Jun 15, 2017

Uh oh!

agramfort commented Jun 15, 2017

Uh oh!

codecov-io commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

larsoner commented Jun 15, 2017 via email

Uh oh!

agramfort commented Jun 15, 2017 via email

Uh oh!

larsoner commented Jun 15, 2017

Uh oh!

larsoner commented Jun 15, 2017

Uh oh!

stfnrpplngr commented Jun 16, 2017

Uh oh!

larsoner commented Jun 16, 2017

Uh oh!

larsoner commented Aug 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stfnrpplngr commented Aug 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dengemann commented Aug 14, 2017 via email

Uh oh!

larsoner commented Aug 14, 2017

Uh oh!

stfnrpplngr commented Aug 21, 2017

Uh oh!

agramfort commented Aug 21, 2017

Uh oh!

larsoner commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Aug 22, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Aug 22, 2017 via email

Uh oh!

larsoner commented Aug 22, 2017

Uh oh!

larsoner commented Aug 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Jun 15, 2017 •

edited

Loading