Introduce lower bounds for ExG peak detection thresholds #9311

hoechenberger · 2021-04-18T09:14:21Z

Fixes #9273

We set 100 * eps as the lower bound.

Fixes mne-tools#9273

mne/preprocessing/tests/test_ecg.py

hoechenberger · 2021-04-18T12:36:27Z

@drammock @larsoner It seems there's a problem with Circle that's unrelated to my PR, could you please have a look?

drammock · 2021-04-18T14:35:24Z

Looks like a deprecation warning from jinja, which will need to be fixed upstream in Sphinx.

…

-------- Original Message --------

On Apr 18, 2021, 07:36, Richard Höchenberger wrote: ***@***.***(https://github.com/drammock) ***@***.***(https://github.com/larsoner) It seems there's a problem with Circle that's unrelated to my PR, could you please have a look? — You are receiving this because you were mentioned. Reply to this email directly, [view it on GitHub](#9311 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAN2AU3ZKFL22NCOU5ZXGNDTJLG5NANCNFSM43EAKTGQ).

hoechenberger · 2021-04-18T15:13:52Z

So should I pin our jinja to an older version perhaps?

drammock · 2021-04-18T19:00:22Z

So should I pin our jinja to an older version perhaps?

we don't directly depend on jinja; it's a sphinx dependency. I think the right thing to do is to open an issue upstream in the sphinx repo, and add this particular deprecation warning to the reset_warnings() function in doc/conf.py so that in the meantime it won't affect our doc builds (I think that's the right place, as opposed to mne/conftest.py... I think conftest is for warnings generated by our tests, and reset_warnings() in conf.py is for warnings generated during doc builds). @larsoner can correct me if I'm wrong.

larsoner · 2021-04-18T19:05:15Z

That's correct!

agramfort · 2021-04-19T06:36:24Z

mne/preprocessing/_peak_finder.py

+    try:
+        eps = 1e2 * np.finfo(x0.dtype).eps
+    except ValueError:  # int dtypes are exact
+        eps = 0.


I don't like try except blocks. They are too implicit. Here i would say that either we support integer inputs and you should check that dtype is int-like or if not you should cast to a float with the same nbytes. See

https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/utils/validation.py#L173

how it's done in sklearn.

I'm now simply casting unless it's already a float, see 20361b4

You okay with that?

mne/preprocessing/_peak_finder.py

larsoner · 2021-04-19T13:28:49Z

doc/conf.py

        'ignore', message="can't resolve package from", category=ImportWarning)
    warnings.filterwarnings(
        'ignore', message='.*mne-realtime.*', category=DeprecationWarning)
+    warnings.filterwarnings(


This was errantly duplicated

Also perhaps you don't need these changes anymore? The latest CircleCI PR doc builds have been green at least

I reverted the changes and now we get the deprecation warnings again and an exception :(

looks like you reverted too much! the jinja glue line was supposed to stay... it's just the accidentally dupllicated mne-realtime line that needed reverting

I thought even the jinja-glue wouldn't be needed since builds are passing on other PRs. But maybe this PR changes something that makes it necessary? Or the caching is making different builds use different versions of Sphinx or something?

version of sphinx is the same between this failed circleCI build and the successful one on #9316 (both use 4.0.0b1). Jinja is different (3.0.0rc1 here versus 3.0.0a1 there)

the jinja error is happening on #9292 now also.

Feel free to port the fix over. Personally I would do it just by adding another line to the DeprecationWarnings list a few lines above

(around line 770 of conf.py)

This reverts commit 640df64.

…rger/issue9273

larsoner · 2021-04-19T15:38:50Z

mne/preprocessing/_peak_finder.py

+        raise ValueError(
+            f'The selected threshold value is too small for peak detection '
+            f'due to inevitable floating-point imprecisions.\nPlease pick a '
+            f'value >= {eps:g} (You passed thresh={thresh})')


I am not convinced this is right. We have to keep in mind eps is relative to 1., it's not an absolute minimum delta between floats (that would be finfo(x0.dtype).tiny, and even that's before going denormalized). For example consider this on main, which seems correct:

>>> import mne >>> import numpy as np >>> x = np.full(1000, 1e-20) >>> x[500] = 2e-20 >>> mne.preprocessing.peak_finder(x) (array([500]), array([2.e-20]))

But we have

>>> (np.max(x) - np.min(x)) / 4 2.5e-21 >>> 1e2 * np.finfo(x.dtype).eps 2.220446049250313e-14

So this PR will find nothing significant.

For EEG/EOG data in V, this might be okay. For data in T or T/M, these hang out around 1e-12 so this can become problematic.

larsoner · 2021-04-23T18:39:13Z

Moving to 0.24 so that whatever solution we come up with we have time to test properly before release

larsoner · 2021-10-22T17:28:59Z

Bumping milestone again

hoechenberger · 2021-10-22T17:34:24Z

Let's keep track of this in #9273.

Closing.

Introduce lower bounds for ExG peak detection thresholds

142fbdd

Fixes mne-tools#9273

hoechenberger requested review from agramfort and larsoner April 18, 2021 09:14

hoechenberger commented Apr 18, 2021

View reviewed changes

mne/preprocessing/tests/test_ecg.py Outdated Show resolved Hide resolved

Remove unnecessary test case

1dffa55

This comment has been minimized.

Sign in to view

hoechenberger added 2 commits April 18, 2021 11:48

Handle eps for exact dtypes

698653d

Add a sanity check for ECG thresholds

aa587a1

agramfort reviewed Apr 19, 2021

View reviewed changes

Cast to float if need be

20361b4

agramfort reviewed Apr 19, 2021

View reviewed changes

mne/preprocessing/_peak_finder.py Outdated Show resolved Hide resolved

hoechenberger added 3 commits April 19, 2021 10:39

Filter deprecation warning

640df64

Review suggestion

32aac0c

flake

7eb97b6

larsoner added this to the 0.23 milestone Apr 19, 2021

larsoner reviewed Apr 19, 2021

View reviewed changes

hoechenberger added 2 commits April 19, 2021 16:51

Revert "Filter deprecation warning"

6164e2f

This reverts commit 640df64.

Merge branch 'main' of github.com:mne-tools/mne-python into hoechenbe…

abc241d

…rger/issue9273

larsoner reviewed Apr 19, 2021

View reviewed changes

larsoner modified the milestones: 0.23, 0.24 Apr 23, 2021

larsoner modified the milestones: 0.24, 1.0 Oct 22, 2021

hoechenberger closed this Oct 22, 2021

hoechenberger deleted the hoechenberger/issue9273 branch December 7, 2021 16:16

hoechenberger restored the hoechenberger/issue9273 branch December 7, 2021 16:16

Uh oh!

Introduce lower bounds for ExG peak detection thresholds #9311

Introduce lower bounds for ExG peak detection thresholds #9311

Uh oh!

Conversation

hoechenberger commented Apr 18, 2021

Uh oh!

Uh oh!

This comment has been minimized.

hoechenberger commented Apr 18, 2021

Uh oh!

drammock commented Apr 18, 2021 via email

Uh oh!

hoechenberger commented Apr 18, 2021

Uh oh!

drammock commented Apr 18, 2021

Uh oh!

larsoner commented Apr 18, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

larsoner commented Apr 23, 2021

Uh oh!

larsoner commented Oct 22, 2021

Uh oh!

hoechenberger commented Oct 22, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants