Remove hardcoded value "BAD_blink" in _interpolate_blinks

[v01dXYZ]

Fixes #13531

Reference issue (if any)

What does this implement/fix?

Additional information

[welcome]

Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴

[scott-huberty]

@v01dXYZ if you sign into CircleCI with your Github account then the build-docs/default jobs will run after the next commit you push here. And also please add a changelog entry to doc/changes/dev/13740.bugfix.rst and add your name and a link to a public profile to doc/changes/names.inc. Just ping me when you are ready for a review!

[scott-huberty]

I think that we can do this without splitting out code into a new function (_annotations_starts_stops_by_idxs).

The _annotations_starts_stops kind parameter excepts a list of str, which are meant to be the annotation descriptions that we want the starts/stops for. So I think we can do something like:

np.unique([annot["description"] for annot in blink_annots]).tolist() or

or
list(set([annot["description"] for annot in blink_annots]))

and pass that to _annots_starts_stops?

[v01dXYZ]

Just to correct a little bit, the argument kind is a list of prefix to match against an annotation description. BUT I understand your point, maybe your prefer to have a selection by description only. Or we can add an argument to it called exact_match or smth else.

[scott-huberty]

the argument kind is a list of prefix to match against an annotation description.

Hmm ok yes I can see how that could cause some trouble...

My reservations are 1) _annotations_starts_stops does some validation checks and now we bypass those without doing any comparable validation.. and 2) I wonder how much value this new function brings (will others find it useful in the future, or does it exist for this one use case here)?

maybe your prefer to have a selection by description only

I mean, that is the expected behavior right? You pass in one or more description names to interpolate_blinks and expect that the data within annotations matching those descriptions get interpolated?

Or we can add an argument to it called exact_match or smth else.

This sounds reasonable to me (and I suspect it could be useful for others in the future!). @larsoner @drammock WDYT about adding an exact_match=False parameter to _annotations_starts_stops, so that e.g. passing _annotations_starts_stops(raw, "my_desc", exact_match=True) doesn't return starts/stops for annotations with descriptions such as "my_desc_2"

[drammock]

For now I've only made suggestions on the changelog, because I want to refocus the discussion a bit. The public API says:

    match : str | list of str
        The description of annotations to interpolate over. If a list, the data within
        all annotations that match any of the strings in the list will be interpolated
        over. If a ``match`` starts with ``'BAD_'``, that part will be removed from the
        annotation description after interpolation. Defaults to ``'BAD_blink'``.

here the word "match" is ambiguous: it could mean "is equal to" or it could mean "match" as in re.match() (which matches at the start of a string). So in my mind, step 1 is clarifying which behavior we want, and then clarifying the docstring accordingly.

If we look at what the functions actually do, the relevant lines are:

mne-python/mne/preprocessing/eyetracking/_pupillometry.py

Line 62 in 82cc6f0

blink_annots = [annot for annot in raw.annotations if annot["description"] in match]

which checks for equality of Annotations.description values and values in match. Then if we actually passed the description strings from blink_annots through here:

mne-python/mne/preprocessing/eyetracking/_pupillometry.py

Line 91 in 82cc6f0

starts, ends = _annotations_starts_stops(raw, "BAD_blink")

(as was likely the intent of @scott-huberty circa July 2024) then blink_annots ends up as kinds here:

mne-python/mne/annotations.py

Lines 1653 to 1657 in 82cc6f0

	idxs = [
	idx
	for idx, desc in enumerate(raw.annotations.description)
	if any(desc.upper().startswith(kind.upper()) for kind in kinds)
	]

Which does a .startswith(). So if a user has a raw with annotation descriptions ['blink', 'blinking_light', ...] and they do interpolate_blinks(raw, match="blink") then the 'blinking_light' spans will get included in the starts, ends... which means that the zip here will get different lengths of objects:

mne-python/mne/preprocessing/eyetracking/_pupillometry.py

Line 94 in 82cc6f0

for annot, start, end in zip(blink_annots, starts, ends):

... and it's very likely that some blinking_light spans will get interpolated, and some blink spans toward the end of the file will not get interpolated.

My recommendation is to take a step back (AKA revert changes), only fix the fact that the descriptions weren't being passed through to _annotations_starts_stops, construct a test case based on my blinking_light example above, and confirm that it indeed fails. Then, make it not fail (which will probably involve some kind of switch to disable the startswith logic in _annotations_starts_stops). Then, clarify the public docstring about what kind of matching is happening.

[drammock]

changelog should always mention the public parts of the API that are changed/fixed:

Suggested change

	``_interpolate_blinks`` now truly takes into account the ``blink_annots`` argument instead of silently using "BAD_blink" annotations only, which is a faulty behaviour.
	Fix bug where :func:`~mne.preprocessing.eyetracking.interpolate_blinks` was overriding the user-passed ``match`` argument with the value ``"BAD_blink"``, by :newcontrib:`Robert Dazi`.

[v01dXYZ]

I'll add some "blinking_light" annotations in the test. (I should have read more carefully the contributing section of the docs, as many things you point out were there...).