ENH: find_blinks (pupil)

[sappelhoff]

• closes [ENH] Blink detection based on pupil size #12808

This PR is currently a draft to discuss an API for a potential find_blinks function that works on eye-tracking data.

Comments welcome -- I would also be happy for pushes to this branch, but if nobody will, then I may eventually get to implementing this, too.

to do

• consensus about API
• add function proper
• add test
• show use in example

[sappelhoff]

@larsoner @britta-wstnr @mscheltienne @drammock @AlexLepauvre @qian-chu @scott-huberty

what do you think of this suggested API?

[drammock]

I don't love the fact that to get all channels annotated you pass either None or []

[sappelhoff]

I know what you mean, and I agree in principle. We had a similar discussion in mne-bids recently:

• deprecate empty list as indicator for marking all channels mne-bids#1307

However, this choice is based on the ch_names parameter in https://mne.tools/stable/generated/mne.Annotations.html

I am open to alternative suggestions.

[scott-huberty]

We could consider having the default behavior be that blink annotations are only associated with eyetrack channels, which would be more consistent with the read_raw_eyelink behavior (see tutorial ). To have blink annotations associated with all channels, the user could pass 'all' (which mimics the pick API).

[qian-chu]

I personallly would prefer this flag to control whetherthe annotation ch_names are set to the chs_src from which that are detected from (default), or []. To me it's a bit unclear in what cases users might want to associate other channels with the blink annotations. Plus, by setting to the source channel names, it might help potential functions for handling blinks detected from both eyes (e.g., whether to merge).

[sappelhoff]

To me it's a bit unclear in what cases users might want to associate other channels with the blink annotations

users may want to associate EEG channels with blinks that are detected from pupil channels, for artifact cleaning.

But as long as this is possible, I do not mind about the exact implementation.

[qian-chu]

users may want to associate EEG channels with blinks that are detected from pupil channels, for artifact cleaning.

Would ch_names=[] be sufficient? I have no experience in oculomotor artifact correction so may have the ask the stupid question of whether those algorithms require annotations with associated EEG channel names?

[sappelhoff]

I think these are details that we can probably decide later on in the process, for now I think that @scott-huberty's proposal has received a lot of 👍, so we should go with this and revisit when it is time to merge.

[drammock]

So we return an Annotations object but don't attach it to the instance? If the instance already has annotations attached, do we update them?

[sappelhoff]

If the instance already has annotations attached, do we update them?

Yes, I had in mind that this should work:

(example 1)

annots = find_blinks(raw, ...)
annots = raw.annotations.copy() + annots 
raw.set_annotations(annots)

The choice of not setting them to inst directly is so that people could do something like:

(example 2)

annots_1 = find_blinks(raw, chs_src=[ch1], description="desc A")
annots_2 = find_blinks(raw, chs_src=[ch2], description="desc B")
raw.set_annotations(annots1 + annots2)

So we return an Annotations object but don't attach it to the instance?

The alternative of the above would be to:

1. support a dict as an argument for description, mapping from chs_src items to descriptions
2. support find_overlaps and overlap_threshold parameters as in read_raw_eyelink, because if we directly attach to inst, we make it hard for users to use the workflow from "example 2" above, and therefore have to offer an alternative

I am open to either (or any) way, as long as we allow workflows that:

1. allow event marking based on source channels
2. allow prescribing event descriptions based on source channels
3. control whether overlapping annotations should be merged or not

My present proposal aims at fulfilling these points without going overboard on parameter number and complexity.

[larsoner]

Generally speaking I think the way most of our annotations functions work is that they return an Annotations instance. It makes it easier to modify however you want before appending to the raw instance you're working with. So for consistency with existing functions and ease of use I think it's a useful pattern.

[scott-huberty]

Thanks for taking the time to put this on the board @sappelhoff ! Just a few comments for now.

[scott-huberty]

This is admittedly a nitpick but I think that instead of using a prepositional phrase (by_), using a term like 'dropout' or 'pupil_dropout' for the method parameter would be more consistent with the other method parameters of MNE (ref 1,ref 2, ref 3, ref 4, ref 5), and so I would prefer to change this to dropout or pupil_dropout.

[scott-huberty]

I imagine that dropout_value could apply to future blink detection algorithms that get implemented in MNE, and so we might want to consider renaming dropout_value to something like blink_value? Just a thought.. Open to other opinions.

[qian-chu]

I also believe that if we are to add more methods such as Hershman, the parameters may be many and take various names. To me it makes more sense to replace dropout_value with params, which, when method='dropout' defaults to dict(dropout_value=None). The accepted dictionary keys will correspond to the method specified, similar to the ICA implementation:

mne-python/mne/preprocessing/ica.py

Lines 250 to 260 in 00c0e21

	method : 'fastica' | 'infomax' | 'picard'
	The ICA method to use in the fit method. Use the ``fit_params`` argument
	to set additional parameters. Specifically, if you want Extended
	Infomax, set ``method='infomax'`` and ``fit_params=dict(extended=True)``
	(this also works for ``method='picard'``). Defaults to ``'fastica'``.
	For reference, see :footcite:`Hyvarinen1999,BellSejnowski1995,LeeEtAl1999,AblinEtAl2018`.
	fit_params : dict | None
	Additional parameters passed to the ICA estimator as specified by
	``method``. Allowed entries are determined by the various algorithm
	implementations: see :class:`~sklearn.decomposition.FastICA`,
	:func:`~picard.picard`, :func:`~mne.preprocessing.infomax`.

[sappelhoff]

agreed!

[scott-huberty]

I think that regardless of method (including Hershman AFAICT), MNE will still need to know what value represents a missing pupil value (e.g. nan or 0) so to me it makes sense to keep this as a dedicated parameter (and this way the parameter is easier to discover for users).

[scott-huberty]

We could consider having the default behavior be that blink annotations are only associated with eyetrack channels, which would be more consistent with the read_raw_eyelink behavior (see tutorial ). To have blink annotations associated with all channels, the user could pass 'all' (which mimics the pick API).

[AlexLepauvre]

Hello everyone, apologies for the radio silence. Myself and @qian-chu got a bit sidetracked over the past few months. I will get back to it next week and follow the recommendation laid out above to bring this PR further :)

[qian-chu]

Thanks for taking the initiative and looping me in! I'm just back from a vacation and also simultaneously developing a package myself so sorry for the limited attention on this implementation. I will try to keep up with the latest development here :)

[qian-chu]

Since blink detection is usually based on pupil data, do you also think it makes more sense to put find_blinks() under _pupillometry.py?

[sappelhoff]

Since blink detection is usually based on pupil data, do you also think it makes more sense to put find_blinks() under _pupillometry.py?

would be fine by me, too.