MRG, ENH: Automatically compute threshold for CTPS ECG detection#7819
MRG, ENH: Automatically compute threshold for CTPS ECG detection#7819agramfort merged 6 commits intomne-tools:masterfrom
Conversation
mne/preprocessing/ica.py
Outdated
|
|
||
| return labels, scores | ||
|
|
||
| def _get_ctps_threshold(self, Pk_threshold=1e-20): |
There was a problem hiding this comment.
| def _get_ctps_threshold(self, Pk_threshold=1e-20): | |
| def _get_ctps_threshold(self, pk_threshold=1e-20): |
you should not have capitals in snake_case variable names if you follow pep8 rule.
There was a problem hiding this comment.
In the paper, Pk was log-transformed to pk: pk = -log10(Pk). Because the threshold value used here was Pk, I wondered if Pk_threshold would be more appropriate than pk_threshold? or maybe significance_level would be better?
|
so Pk is a p-value?
… |
|
Pk is a value to evaluate whether an IC is ECG-related.
|
|
I would just call it threshold and explain in the docstring to what it
corresponds in the paper.
|
* ENH: Test deprecation warning for ctps threshold * DOC: Update find_bads_ecg documentation
larsoner
left a comment
There was a problem hiding this comment.
git grep find_bads_ecg suggests you'll also need to update tutorials/preprocessing/plot_40_artifact_correction_ica.py, preferably to use the new threshold='auto' option
mne/preprocessing/tests/test_ica.py
Outdated
| assert_equal(len(scores), ica.n_components_) | ||
|
|
||
| idx, scores = ica.find_bads_ecg(evoked, method='correlation') | ||
| idx, scores = ica.find_bads_ecg(evoked, method='correlation', threshold=3) |
There was a problem hiding this comment.
Can this be changed to threshold='auto' and still work?
There was a problem hiding this comment.
Thanks for reminding, I've checked and updated the influenced tests and examples.
|
good to go?
… |
* ENH: Tests for ECG detection uses threshold='auto' instead of None
|
BTW, the computed threshold in |
|
@yh-luo you will need to update latest.inc file to get credit and document this improvement. thanks |
|
|
||
| - The function ``mne.channels.read_dig_captrack`` will be deprecated in version 0.22 in favor of :func:`mne.channels.read_dig_captrak` to correct the spelling error: "captraCK" -> "captraK", by `Stefan Appelhoff`_ | ||
|
|
||
| - The ``threshold`` argument in :meth:`mne.preprocessing.ICA.find_bads_ecg` defaults to ``None`` in version 0.21 but will change to ``'auto'`` in 0.22 by `Yu-Han Luo`_ |
There was a problem hiding this comment.
you need you add your name to names.inc too
There was a problem hiding this comment.
thank you! I had added my name to names.inc in previous PR, so I did not in this one. Locally built documentation works as expected.
|
thx @yh-luo ! |
* upstream/master: MRG: Add support for foreground in _Brain (mne-tools#7843) MRG, MAINT: Change default role in conf.py (mne-tools#7841) MRG, ENH: Support n_col keyword in ica.plot_score (mne-tools#7825) add icons to source dist (mne-tools#7840) Add CZI to list of funders (mne-tools#7839) DOC: added reference to sesameeg package (mne-tools#7835) MRG, ENH: Automatically compute threshold for CTPS ECG detection (mne-tools#7819) MAINT: Show how picks work for planars (mne-tools#7833) Clearer info docstring (mne-tools#7832) MRG, ENH: Add estimation method legend (mne-tools#7830) Remove double spaces (mne-tools#7822) add troubleshooting message about OpenGL [skip travis] (mne-tools#7827) fix links [skip travis] (mne-tools#7826)
Reference issue
Implementation in #7815
Additional information
After a deprecation cycle, TODO part will replace the current default value 0.25.
If that's ok, I will update
latest.inc