[FIX] Add tol parameter to events_from_annotations by rcmdnk · Pull Request #12324 · mne-tools/mne-python

rcmdnk · 2023-12-25T12:19:17Z

Reference issue

What does this implement/fix?

This change adds a tol (tolerance) parameter to annotations.events_from_annotations.

In the function, the following equation is affected by rounding errors:

good_events = annot_offset - _onsets >= chunk_duration

Even if annot_offset - _onsets[x] equals chunk_duration through manual calculation, annot_offset - _onsets[x] could be slightly smaller than chunk_duration.

To address this issue, the tol parameter is introduced:

good_events = annot_offset - _onsets >= chunk_duration - tol

Additional information

It may be advisable to use a non-zero default value for tol. In this context, using a parameter fraction like epsilon may be better.
The tol value should be calculated as:

tol = (1. / raw.info['sfreq']) * epsiron

And a default value of epsilon=1e-5 seems appropriate.

However, this default value alters the behavior, so it should be properly communicated to users.

agramfort · 2023-12-27T18:03:04Z

the issue you report in #12321 suggests it's a silent bug that can happen quiet easily. I fear that with just the docstring you have here, users will unlikely do the right thing. Do you see a way to have a better default? Do you see a case where using tol=1e-15 can lead to some unexpected behavior?

do you think there is bug in this tutorial https://mne.tools/stable/auto_tutorials/clinical/60_sleep.html ?

rcmdnk · 2023-12-28T00:48:26Z

@agramfort
Thanks for the comment.

The example case needs at least tol of 1e-11:

>>> onset=32760.12
>>> duration=30.0
>>> chunk_duration=30.0
>>> onset + duration - onset - chunk_duration
-3.637978807091713e-12

I think something like 1e-8 is enough large for the tolerance
and also enough small comparing to the interval of data (for frequency up to O(100Hz) at most).

I updated the default value of tol = 1e-8.

For the tutorial, Both meas_date of Raw (then it is the orig_time of the annotation) and all onset of Annotations are not decimals and it is not affected by this issue.

agramfort · 2023-12-28T17:42:53Z

Ok I fully get the issue now. Note that this reminds me of:

>>> np.arange(7.8, 8.4, 0.05)
array([7.8 , 7.85, 7.9 , 7.95, 8.  , 8.05, 8.1 , 8.15, 8.2 , 8.25, 8.3 ,
       8.35, 8.4 ])

basically when you have a float step size you have rounding errors. Here it's created cause there is a loss of accuracy with the meas_date but it feels it can happen also without.

what makes me less supportive of this change is that if you use the public API ie raw.set_annotations(annot) then the issue does not seem to happen.

agramfort · 2023-12-28T17:42:01Z

mne/tests/test_annotations.py

+    with raw.info._unlock(check_after=True):
+        raw.info["meas_date"] = meas_date
+    annot = Annotations([32730.12, 32760.12, 32790.12], 30.0, ["0", "1", "2"], 0)
+    raw._annotations = annot


raw.set_annotations(annot)

is the recommended way. If you do this then the issue does not occur on my end.

agramfort · 2024-01-01T16:21:12Z

@rcmdnk sorry my last comment about using raw.set_annotations had not been sent.

rcmdnk · 2024-01-15T11:49:36Z

@agramfort,

Apologies for the delayed response.

To clarify, the previous test would not have encountered any loss if set_annotations were used, because the orig_time of the annotation is modified within this function. The reason I opted not to use set_annotations is due to my choice of 32730.12 as the onset value. Starting from 0 with this onset value would require a significantly large dataset (on the order of 10,000,000).

However, if the meas_date of the raw data and the orig_time of the annotations are identical, using set_annotations won't alter the onsets, and the original issue persists. Furthermore, if the duration includes a decimal value, the same problem can arise.

I've simplified the test to demonstrate that using set_annotations with a tol value can effectively address the issue as anticipated.

agramfort

ok it seems legit !

a couple of remaining nitpicks.

sorry for the slow reaction time ...

agramfort · 2024-02-04T20:06:32Z

doc/changes/devel/12324.bugfix.rst

@@ -0,0 +1 @@
+Add ``tol`` parameter to :meth:`mne.events_from_annotations`, by `Michiru Kaneda`_


can you be more explicit for this sentence? it's written as an enhancement and not really a bug fix. Something like

Add tol parameter to :meth:mne.events_from_annotations so that ... when using chunk_duration != None, by Michiru Kaneda_

Here is an update:
7dd7375

agramfort · 2024-02-04T20:07:02Z

mne/annotations.py

+        The tolerance which is used to check if the calculated onsets have the
+        enough duration to the end of the annotation. If the duration from the
+        onset to the end of the annotation is smaller than ``chunk_duration``
+        minus ``tol``, the onset will be discarded.


can you clarify that this tol parameter is only useful when chunk_duration != None?

fixed:
8b146a2

larsoner · 2024-02-06T17:01:52Z

@rcmdnk sorry for the hassle but it looks like there is a conflict, could you git fetch upstream && git merge upstream main and fix the conflicts (or rebase if you prefer)?

Adjust tolerance parameter in events_from_annotations to 1e-8 Update annotation tests to reflect new tolerance parameter of 1e-8

…ons` to include usage details

…from_annotations` docstring

rcmdnk · 2024-02-07T03:07:28Z

@larsoner
ok, the branch is rebased on main now.

drammock

LGTM, just 2 suggestions. +1 for merge after those are addressed

drammock · 2024-02-07T18:04:18Z

mne/annotations.py

+        The tolerance which is used to check if the calculated onsets have the
+        enough duration to the end of the annotation when``chunk_duration`` is
+        not ``None``. If the duration from the onset to the end of the
+        annotation is smaller than ``chunk_duration`` minus ``tol``, the onset
+        will be discarded.


minor wording change; I think this makes the purpose clearer (but please confirm that I've understood the purpose correctly!)

Suggested change

The tolerance which is used to check if the calculated onsets have the

enough duration to the end of the annotation when``chunk_duration`` is

not ``None``. If the duration from the onset to the end of the

annotation is smaller than ``chunk_duration`` minus ``tol``, the onset

will be discarded.

The tolerance used to check if a chunk fits within an annotation

when ``chunk_duration`` is not ``None``. If the duration from a

computed chunk onset to the end of the annotation is smaller than

``chunk_duration`` minus ``tol``, the onset will be discarded.

Thanks, this change is fine with me.

drammock · 2024-02-07T18:18:28Z

mne/tests/test_annotations.py

    assert raw.first_samp == event_latencies[0, 0]


+def test_events_from_annot_with_tolerance():


this test is ripe for a @pytest.mark.parametrize decorator. Do you know how to do that? If not I can push the change... something like

@pytest.mark.parametrize( "use_rounding,tol,shape,idx", ( pytest.param(True, 0, (2, 3), [202, 402], id="rounding-notol"), pytest.param(True, 1e-8, (3, 3), [202, 302, 402], id="rounding-tol"), pytest.param(False, 0, (3, 3), [202, 302, 402], id="norounding-notol"), pytest.param(False, 1e-8, (3, 3), [202, 302, 401], id="norounding-tol"), ) ) def test_events_from_annot_with_tolerance(use_rounding, tol, shape, idx): """.""" # ...setup code here events, _ = events_from_annotations( raw, event_id={"0": 0, "1": 1, "2": 2}, chunk_duration=1, use_rounding=use_rounding, tol=tol, ) assert events.shape == shape assert (events[:, 0] == idx).all()

Yes, thank you for the suggestion; it's a smarter approach. The test has been updated to use parametrize.

update tests events_from_annotations with parameterize

drammock · 2024-02-08T15:08:43Z

thanks @rcmdnk!

rcmdnk requested review from agramfort, dengemann, drammock and larsoner as code owners December 25, 2023 12:19

rcmdnk mentioned this pull request Dec 25, 2023

Add tolerance parameter to events_from_annotations to #12321

Closed

rcmdnk force-pushed the fix_events_from_annotations branch 2 times, most recently from 6a3e0e5 to 5059033 Compare December 27, 2023 00:31

rcmdnk changed the title ~~[ENH] Add tol parameter to events_from_annotations~~ [BUG] Add tol parameter to events_from_annotations Dec 28, 2023

rcmdnk force-pushed the fix_events_from_annotations branch from 3f9a260 to 1278319 Compare December 28, 2023 00:49

rcmdnk changed the title ~~[BUG] Add tol parameter to events_from_annotations~~ [FIX] Add tol parameter to events_from_annotations Dec 28, 2023

agramfort mentioned this pull request Dec 28, 2023

Annotation timing info loss due to datetime.timedelta precision limit #12327

Open

agramfort reviewed Jan 1, 2024

View reviewed changes

rcmdnk force-pushed the fix_events_from_annotations branch 3 times, most recently from b9cd0c1 to 1780981 Compare January 15, 2024 11:20

agramfort reviewed Feb 4, 2024

View reviewed changes

rcmdnk added 8 commits February 7, 2024 10:16

Add tol parameter to events_from_annotations

d56357d

Add GitHub PR number in devel.rst

2817005

make 12324.newfeature.rst

1334a0c

fix meth reference name

b98fdd3

Rename changelog from 12324.newfeature.rst to 12324.bug.rst

3bdb85e

Adjust tolerance parameter in events_from_annotations to 1e-8 Update annotation tests to reflect new tolerance parameter of 1e-8

Update test simplify

abc3e34

Update documentation for tol parameter in `mne.events_from_annotati…

534fa2f

…ons` to include usage details

Clarify tolerance condition chunk_duration is not None in events_…

727fe92

…from_annotations` docstring

rcmdnk force-pushed the fix_events_from_annotations branch from 8b146a2 to 727fe92 Compare February 7, 2024 01:16

fix document

edd3909

drammock reviewed Feb 7, 2024

View reviewed changes

update document for tol of events_from_annotations

a533b8a

update tests events_from_annotations with parameterize

drammock approved these changes Feb 8, 2024

View reviewed changes

drammock merged commit 6857f10 into mne-tools:main Feb 8, 2024

rcmdnk deleted the fix_events_from_annotations branch February 14, 2024 11:46

snwnde pushed a commit to snwnde/mne-python that referenced this pull request Mar 20, 2024

[FIX] Add tol parameter to events_from_annotations (mne-tools#12324)

8ace10a

		@@ -0,0 +1 @@
		Add ``tol`` parameter to :meth:`mne.events_from_annotations`, by `Michiru Kaneda`_

		assert raw.first_samp == event_latencies[0, 0]


		def test_events_from_annot_with_tolerance():

Uh oh!

Conversation

rcmdnk commented Dec 25, 2023

Reference issue

What does this implement/fix?

Additional information

Uh oh!

agramfort commented Dec 27, 2023

Uh oh!

rcmdnk commented Dec 28, 2023

Uh oh!

agramfort commented Dec 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jan 1, 2024

Uh oh!

rcmdnk commented Jan 15, 2024

Uh oh!

agramfort left a comment

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 Feb 6, 2024

Uh oh!

rcmdnk commented Feb 7, 2024

Uh oh!

drammock left a comment

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!

drammock commented Feb 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants