Skip to content

Conversation

@hoechenberger
Copy link
Member

@hoechenberger hoechenberger commented Feb 19, 2020

What does this implement/fix?

Removes the mne.preprocessing.ica.run_ica() function. All it does it instantiate an ICA object and then run an artifact detection. However, the same can be achieved by using ICA.detect_artifacts().
Deprecates mne.preprocessing.ica.run_ica() in favor of ICA.detect_artifacts()

@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

Merging #7342 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7342      +/-   ##
==========================================
- Coverage   89.99%   89.94%   -0.06%     
==========================================
  Files         450      450              
  Lines       80659    81587     +928     
  Branches    12943    13003      +60     
==========================================
+ Hits        72589    73382     +793     
- Misses       5244     5385     +141     
+ Partials     2826     2820       -6     

@hoechenberger hoechenberger changed the title Remove run_ica() Deprecate run_ica() Feb 19, 2020
@larsoner
Copy link
Member

git grep run_ica tells me that examples/preprocessing/plot_ica_comparison.py should be updated, too

@hoechenberger
Copy link
Member Author

git grep run_ica tells me that examples/preprocessing/plot_ica_comparison.py should be updated, too

No, it defines its own run_ica function which it then calls with different parameter values to create different plots

@larsoner
Copy link
Member

Hah! fair enough

@hoechenberger
Copy link
Member Author

@larsoner @drammock I believe I've addressed all of your comments.

Copy link
Member

@larsoner larsoner left a comment

Choose a reason for hiding this comment

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

LGTM +1 for merge once CIs come back happy

@dengemann
Copy link
Member

dengemann commented Feb 19, 2020 via email

@drammock
Copy link
Member

The Travis failure is a general timeout error at the CodeCov step, seems unrelated to this PR (@larsoner have you seen that before? Seems odd). Azure is "Windows fatal exception: caccess violation" which is also unrelated (though is happening a lot lately, should be investigated).

@drammock drammock merged commit e3c4474 into mne-tools:master Feb 19, 2020
@hoechenberger hoechenberger deleted the run_ica branch February 19, 2020 22:01
@larsoner
Copy link
Member

@drammock the Travis timeouts at various stages are just numpy/numpy#15580

The Azure failures I was hoping to fix with #7295 but hasn't quite done the trick

AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Deprecate Deprecate mne.preprocessing.run_ica()

* Update mne/preprocessing/tests/test_ica.py

Co-Authored-By: Daniel McCloy <dan@mccloy.info>

* Refactor tests

* Update docstring

Co-authored-by: Daniel McCloy <dan@mccloy.info>
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Deprecate Deprecate mne.preprocessing.run_ica()

* Update mne/preprocessing/tests/test_ica.py

Co-Authored-By: Daniel McCloy <dan@mccloy.info>

* Refactor tests

* Update docstring

Co-authored-by: Daniel McCloy <dan@mccloy.info>
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.

4 participants