Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ENH: find_blinks (pupil) #12946

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+68 −3
Draft

ENH: find_blinks (pupil) #12946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mne/preprocessing/eyetracking/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
# License: BSD-3-Clause
# Copyright the MNE-Python contributors.

from .eyetracking import set_channel_types_eyetrack, convert_units
from .eyetracking import set_channel_types_eyetrack, convert_units, find_blinks
from .calibration import Calibration, read_eyelink_calibration
from ._pupillometry import interpolate_blinks
from .utils import get_screen_visual_angle
63 changes: 61 additions & 2 deletions mne/preprocessing/eyetracking/eyetracking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,68 @@
from ...epochs import BaseEpochs
from ...evoked import Evoked
from ...io import BaseRaw
from ...utils import _check_option, _validate_type, logger, warn
from ...utils import _check_option, _validate_type, logger, verbose, warn
from .calibration import Calibration
from .utils import _check_calibration


@verbose
def find_blinks(
inst,
*,
chs_src=None,
method="by_dropout",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dropout_value=None,
description="BAD_blink",
chs_dest=None,
verbose=None,
):
"""Find blinks in eye-tracking data and create corresponding annotations.

Parameters
----------
inst : instance of Raw
The data instance to use for finding blinks. Must contain pupil channels.
chs_src : list | None
A list of channel names that will be used to find blinks. None (default) will
result in selecting all channels of type ``pupil``. See Notes for more
information.
method : str
Which method to use to find blinks in ``inst``. Currently the only supported
method is ``'by_dropout'`` (default), which uses the value specified via
``dropout_value`` to identify blinks.
dropout_value : float | None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Which value in the data denotes a dropout. In Eyelink data, this is ``0``,
whereas for other eye-tracking data this may be ``np.nan``, or something else.
Defaults to None, which sets the ``dropout_value`` to ``np.nan``.
description : str
Which description to use for the blink annotations. Defaults to ``'BAD_blink'``.
chs_dest : list | None
A list of channel names that will be associated with the blink annotations.
None (default) and passing an empty lists will associate each blink annotation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

Copy link
Contributor

@scott-huberty scott-huberty Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

with all channels in ``inst``.
%(verbose)s

Returns
-------
annots : mne.Annotations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

The annotations objects containing blink annotations.

Notes
-----
If multiple channels are used to find blinks in ``inst``, resulting overlapping
blink annotations are always merged over all channels. That is, if a left and a
right pupil channel would be used for blink detection, and each on their own would
produce overlapping blink annotations with onsets ``[1.5, 1.6]`` and durations
``[0.2, 0.3]``, respectively, then passing both channels into this function will
result in a single blink annotation with onset ``1.5`` and duration ``0.4``.
Note that this corresponds to the minimum onset and the maximum offset between
the two annotations.
"""
logger.info("Found blinks")
return


# specific function to set eyetrack channels
def set_channel_types_eyetrack(inst, mapping):
"""Define sensor type for eyetrack channels.
Expand Down Expand Up @@ -168,7 +225,8 @@ def _convert_deg_to_rad(array):
return array * np.pi / 180.0


def convert_units(inst, calibration, to="radians"):
@verbose
def convert_units(inst, calibration, to="radians", verbose=None):
"""Convert Eyegaze data from pixels to radians of visual angle or vice versa.

.. warning::
Expand All @@ -191,6 +249,7 @@ def convert_units(inst, calibration, to="radians"):
(in pixels).
to : str
Must be either ``"radians"`` or ``"pixels"``, indicating the desired unit.
%(verbose)s

Returns
-------
Expand Down
6 changes: 6 additions & 0 deletions mne/preprocessing/eyetracking/tests/test_eyetracking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,15 @@

import mne
from mne._fiff.constants import FIFF
from mne.preprocessing.eyetracking import find_blinks
from mne.utils import _record_warnings


def test_find_blinks():
"""Test creating blink annotations."""
find_blinks


def test_set_channel_types_eyetrack(eyetrack_raw):
"""Test that set_channel_types_eyetrack worked on the fixture."""
assert eyetrack_raw.info["chs"][0]["kind"] == FIFF.FIFFV_EYETRACK_CH
Expand Down
Loading