BUG: don't reset ICA.labels_ #5963
Conversation
…partially visualising these components
new function find_bads_ref identifies bad components that correlate with reference channel derived components
Codecov Report
@@ Coverage Diff @@
## master #5963 +/- ##
========================================
+ Coverage 88.79% 89.1% +0.3%
========================================
Files 401 401
Lines 72721 74210 +1489
Branches 12158 12619 +461
========================================
+ Hits 64575 66126 +1551
+ Misses 5218 5197 -21
+ Partials 2928 2887 -41 |
|
good to go? |
|
Fine by me. |
|
@larsoner asked for a test no? also @jshanna100 you should not open PRs from your master branch |
|
This should work, though I can't be 100% sure at the moment. Matplotlib has suddenly decided to fail for all of the tests, and is throwing out a million errors, so it's hard to diagnose. Going on holiday tomorrow, so can't sort it out right now. My apologies for that... |
|
In the first run there are PEP8 problems https://travis-ci.org/mne-tools/mne-python/jobs In the second run it times out because the test takes longer than 30 sec. Can you shorten the time of this test as much as possible? |
|
Ideally a test for this sort of thing could be a few lines added to an existing test, adding almost no time to the run. If new ICA fitting is really required, limiting channel picks and amount of data fit helps. See how we do it in other tests |
|
Ok, though I will not be able to tend to this myself until the week after next. Part of what needs to be tested is that the labelling for all three find_bads_ play together well, which, AFAICT, means that if it is to integrate with an already existing test it would have to be test_ica_ctf - this uses the only test dataset with EOG, ECG, and reference channels. Including find_bads_ref means that an extra ICA fit on reference channels is unavoidable, though this adds very little time. In the case where we keep test_ica_labels as a separate test, the number of components on the main ICA fit could be significantly reduced, which may fix the timeout. I can't figure out from the output what specifically the PEP8 problem is, and the link posted above doesn't go anywhere. |
|
I'm fine just adding it to Here is a better link, toward the bottom https://travis-ci.org/mne-tools/mne-python/jobs/496748047#L3209 |
|
Important fix, thanks for checking this @jshanna100 . |
|
Should be good now. |
|
@jshanna100 does the test take much longer now? |
|
On a moderately fast desktop computer, it adds about 1 second. |
mne/preprocessing/tests/test_ica.py
Outdated
| icacomps = icarf.get_sources(raw) | ||
| # rename components so they are auto-detected by find_bads_ref | ||
| for c in icacomps.ch_names: | ||
| icacomps.rename_channels({c: 'REF_' + c}) |
There was a problem hiding this comment.
icacomps.rename_channels({c: 'REF_' + c for c in icacomps.ch_names})
would be nicer and faster.
agramfort
left a comment
agramfort
left a comment
There was a problem hiding this comment.
besides LGTM. Maybe a what's new update if it fixes something from 0.17 release?
|
I had a few minor nitpicks so I pushed them along with @agramfort's suggestion. ICA on ref is part of the 0.18 cycle so no need for Will merge once CI's come back happy (and will fix them if I broke them), thanks @jshanna100 |
|
Thanks @jshanna100 |
* bare bones functionality for doing ICA on MEG reference channels and partially visualising these components * bypassing topography, incomplete work on correlative functions * woops * ICA accepts reference channels, topography functions are skipped, new function find_bads_ref identifies bad components that correlate with reference channel derived components * Solved various backwards compatibility issues. * new private function _find_bads_ch now underlies find_bads_ecg/eog/ref * should pass ica_test.py now * fixed documentation formatting * fix copy paste error in notes * FIX: Minor fixes * FIX: Order * fix ICA.label_ bug * restore individual channel info ICA.labels_ * new test for checking ICA.labels_ * rolled ICA.labels_ test into test_ica_ctf. * ENH: Fixes for Alex
4 participants
Bug from #5807. Every time a find_bads_eog/ref/ecg method was used, it reinitiated everything in ICA.labels_. Now they accumulate across multiple applications of these methods as they should.