Skip to content
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

Add Corrmap #1795

Closed
wants to merge 65 commits into from
Closed

Conversation

jona-sassenhagen
Copy link
Contributor

Implementation of Corrmap; website, journal paper.

Corrmap is a procedure for semi-automatic selection of maps, especially for selecting common ICs for a number of subjects. The user inputs a list of fitted ICAs (such as for all of the subjects of a study) and denotes a template IC (such as a blink IC). The Corrmap algorithm then determines the best-fitting ICs for all ICAs and adds them to the ICAs in a specified field.

Here is a gist.

The routine should basically work and be somewhat robust. I'm open to any feedback.
This is not a translation of the original MATLAB program, but an attempt of recreating the behaviour as described in the journal paper.

Currently, the routine only takes ICs, but in the long term, it could be expanded to select any sets of matching topographies, such as for basic microstate analysis.

I'm not sure how a test could be built as the algorithm requires multiple IC decompositions.

@agramfort
Copy link
Member

@jona-sassenhagen thanks heaps for sharing. Have you worked with this code on a few datasets? My logic is that if it's still early, it's better to iterate quickly on this code on your end and merge it after it proved useful on a few datasets.

my 2c

@jona-sassenhagen
Copy link
Contributor Author

Here is another dataset, this time 4 participants with 64 channels.

Fastica
AMICA

Good results, at least in the second case. It does not perform well in the first because the decompositions are not very good.
So this is also an example of how AMICA, compared to Fastica, can give very good decompositions without any prior data cleaning or parameter tuning, and how it avoids splitting components. (It is possible Fastica would be much better with e.g. rejections enabled, I have not tried to fine-tune Fastica.)

I will run another test soon.

@agramfort
Copy link
Member

agramfort commented Feb 12, 2015 via email

@dengemann
Copy link
Member

@jona-sassenhagen this looks pretty cool! Let me know when you need some reviewing.

@jona-sassenhagen
Copy link
Contributor Author

Another demonstration: full study of 20 subjects. http://nbviewer.ipython.org/gist/jona-sassenhagen/ec915a7e9abda1e2828b
Completely missed the blink IC for 1 subject and chose the wrong IC for another. Not bad.
Also includes example of corrected vs. uncorrected data.

@dengemann , can you indeed take a quick look at the code itself? No details for now, just tell me if it generally works with the way MNE is laid out.

name="marked_ics",
verbose_return=False,
plot=True,
inplace=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jona-sassenhagen no reason to use line breaks here
Also note that we use @verbose decorater together with a verbose keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm deleting the verbose_return thing, it's dumb.

@dengemann
Copy link
Member

@jona-sassenhagen I would put it in ICA, not in bads.

@dengemann
Copy link
Member

@jona-sassenhagen apart from my comments looks like a good start to me!

Integrated criticism by dengeman
@agramfort
Copy link
Member

I would use the term labels as what you're doing is labeling components. So ica.labels

also make sure you check pep8 compliance of your file.

And it would be really great to have at least a smoke test.

@jona-sassenhagen
Copy link
Contributor Author

Especially if you repeatedly do the same analysis, run ICA with a fixed random state, and store the correct templates.

@dengemann
Copy link
Member

Ok. Just wanted to make sure all we have here is needed.

@jona-sassenhagen
Copy link
Contributor Author

Travis:

Test file downloading ... ERROR
======================================================================
ERROR: Test file downloading

Is this my fault?

@larsoner
Copy link
Member

No, sometimes we get those failures

@jona-sassenhagen
Copy link
Contributor Author

Can I restart Travis somehow? It's all urlib errors.

@larsoner
Copy link
Member

You have to have commit rights on the repo to restart, but I restarted it for you

@jona-sassenhagen
Copy link
Contributor Author

Thanks @Eric89GXL

@jona-sassenhagen
Copy link
Contributor Author

Again URLError: and TimeoutError: [Errno 110] Connection timed out ...

@larsoner
Copy link
Member

Just ignore those for now -- if those are the only errors, is this otherwise ready for merge? We might end up having to merge some red PRs today because of connectivity issues, once we make sure that the only problems are connectivity errors.

@dengemann
Copy link
Member

@jona-sassenhagen @Eric89GXL this PR induces 9% of coverage loss in ica.py ...

@jona-sassenhagen
Copy link
Contributor Author

Can we check somewhere what lines are not being tested? I'm at pretty sure corrmap isn't even 10% of the ica code 

On Sat, Apr 18, 2015 at 11:29 AM -0700, "Denis A. Engemann" notifications@github.com wrote:

@jona-sassenhagen @Eric89GXL this PR induces 9% of coverage loss in ica.py ...


Reply to this email directly or view it on GitHub.

@larsoner
Copy link
Member

You can't trust the coveralls report because coveralls is only called when a build succeeds

@larsoner
Copy link
Member

...so you have to check the changes locally. Can you take a look and compare master to your PR @jona-sassenhagen ?

@dengemann
Copy link
Member

I checked locally

Nostests --with-coverage

On 18 Apr 2015, at 21:08, Eric Larson notifications@github.com wrote:

...so you have to check the changes locally. Can you take a look and compare master to your PR @jona-sassenhagen ?


Reply to this email directly or view it on GitHub.

@coveralls
Copy link

coveralls commented Apr 19, 2015

Coverage Status

Coverage decreased (-0.2%) to 90.017% when pulling 877a4db on jona-sassenhagen:patch-3 into 4ae85b5 on mne-tools:master.

@jona-sassenhagen
Copy link
Contributor Author

For some reason we had plot=False for all tests. Coverage is much better now. Basically the lines it's not covering right now are some of the exceptions raised by the various failure options.

I'm considering adding the ability to deal with heterogenous IC objects, e.g. when ICA is called on bad channels.

@jona-sassenhagen
Copy link
Contributor Author

@dengemann like this?

@jona-sassenhagen
Copy link
Contributor Author

Would be very cool to have ICA interpolation ... I'm playing around with _interpolate_bads_eeg, but I'm too bad at lin alg to really make it work.

@dengemann dengemann mentioned this pull request Apr 20, 2015
@dengemann dengemann closed this Apr 20, 2015
@dengemann
Copy link
Member

@jona-sassenhagen closing in favor of #1985.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants