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.

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] BEP 020 Eye Tracking #1128

Open
wants to merge 180 commits into
base: master
Choose a base branch
from
Open

[ENH] BEP 020 Eye Tracking #1128

wants to merge 180 commits into from

Conversation

mszinte
Copy link

@mszinte mszinte commented Jun 15, 2022

Here is the specifications of the BEP 020 about eye tracking.

  • it follows the main discussion initiated on a google document.
  • it includes the different modification the group of maintainers suggested to us during our zoom meeting.
  • it includes the macros as used in other modality specific extensions
  • it includes links toward dataset examples.

Note

We meet regularly and everyone is welcome :
Next meeting October 3Rd 2024 4pm CET on zoom.

Note that if you consider joining but this time or day doesn't suits you, reach me (@mszinte) and I will arrange another appointment.

Notes of last meeting

Chat and discussions also happening on matrix

We are currently drafing a companion paper for this BEP, feel free to participate (GoogleDoc)

Issues for:


  • implement macros
    • for filename templates ?
    • for examples ?
    • for metadata table
  • add contributors to the wiki (so they can be added to the contributors page)
  • end docmentation
  • update examples
  • update validator
  • update list of contributors via the github wiki

@mszinte mszinte requested a review from tsalo as a code owner June 15, 2022 12:09
correction of text
@sappelhoff sappelhoff added the BEP label Jul 12, 2022
@sappelhoff
Copy link
Member

(NOTE: I'll cross-post this message across several BEP threads)

Hi there, just a quick notification that we have just merged #918 and it may be interesting to look at the implications for this BEP.

We are introducing "BIDS URIs", which unify the way we refer to and point to files in BIDS datasets (as opposed to "dataset-relative" or "subject-relative" or "file-relative" links).

If the diff and discussion in the PR is unclear, you can also read the rendered version: https://bids-specification.readthedocs.io/en/latest/02-common-principles.html#bids-uri

Perhaps there are things in the BEP that need adjusting now, but perhaps also not -- in any case it's good to be aware of this new feature!

Let me know if there are any questions, comments, or concerns.

mkdocs.yml Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/schema/metadata/AOIDefinition.yaml Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
src/04-modality-specific-files/10-eye-tracking.md Outdated Show resolved Hide resolved
@tsalo tsalo changed the title [ENH] Bep020 [ENH] BEP 020 Eye Tracking Aug 24, 2022
&lt;<a href="../glossary.html#data_type-common_principles">datatype</a>&gt;/
&lt;matches&gt;[_<a href="../appendices/entities.html#recording">recording</a>-&lt;<a href="../glossary.html#label-common_principles">label</a>&gt;]_&lt;<a href="../glossary.html#physio-suffixes">physio</a>|<a href="../glossary.html#physioevents-suffixes">physioevents</a>&gt;<a href="../glossary.html#json-extensions">.json</a>
&lt;matches&gt;[_<a href="../appendices/entities.html#recording">recording</a>-&lt;<a href="../glossary.html#label-common_principles">label</a>&gt;]_&lt;<a href="../glossary.html#physio-suffixes">physio</a>|<a href="../glossary.html#physioevents-suffixes">physioevents</a>&gt;<a href="../glossary.html#tsvgz-extensions">.tsv.gz</a>
</code></pre></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK shouldn't be HTML here -- I can't read it... Use MACRO as below or demonstrate on a simple Text example?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Problem is that that matches is not in the glossary so we can't have macro yet.

Opened this so we can have it in the glossary and then after make it easier to MACRO the 💩 out of this.
#1781

Copy link
Collaborator

Choose a reason for hiding this comment

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

See also: #1128 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAIK shouldn't be HTML here -- I can't read it... Use MACRO as below or demonstrate on a simple Text example?

Yes, we're using this to set some expectation for the macros to deal with this.

"Columns": ["onset", "duration", "trial_type", "blink", "message"],
"Description": "Messages logged by the measurement device",
"ForeignIndexColumn": "timestamp",
"blink": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

shameless plug: this is the situation I would like to avoid in

where I guess such file would look like

{
    "Description": "Messages logged by the measurement device",
    "ForeignIndexColumn": "timestamp",
    "Columns": {
       "onset": { "Index": 1},
       "duration": {"Index": 2},
       "trial_type": {
           "Description": "Event type as identified by the eye-tracker's model (either 'n/a' if not applicabble, 'fixation', or 'saccade')."
           "Index": 3}, 
       "blink": {
         "Description": "One indicates if the eye was closed, zero if open."
         "Index" : 4},
       "message": {
         "Description": "String messages logged by the eye-tracker.",
         "Index": 5}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm +1 on this and the solution.

However, I understand that can only be implemented in BIDS 2.0 because would introduce backward breaking changes, right?

If so, I think this is something we cannot resolve here :(

@effigies
Copy link
Collaborator

Sorry, I haven't been following this, but if adding a timestamp column is an issue, have you looked at blood for prior art, which has a time column in seconds from scan start?

@oesteban
Copy link
Collaborator

Sorry, I haven't been following this, but if adding a timestamp column is an issue, have you looked at blood for prior art, which has a time column in seconds from scan start?

It is not an issue (other than whether we are correctly configuring the schema #1128 (comment) and allowing an optional column to be placed first in the file when there are two mandatory columns after)

It seems that most of the ET devices do generate a timestamp/time column, so that would not be problematic at all.

Comment on lines +268 to +269
Some metadata of the `StimulusPresentation` object become REQUIRED with the presence of
[eye-tracking data](physiological-recordings.md#eye-tracking)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As mentioned yesterday in the meeting: only for on-screen eyetracking.

Makes the validation of those a bit more complicated (one extra If).

I wonder if we should not have a similar warning in the eyetracking section, or at least a link to the task page.

@oesteban oesteban requested review from tsalo and rwblair May 29, 2024 08:58
@oesteban
Copy link
Collaborator

oesteban commented May 29, 2024

@rwblair can we get your input about allowing/validating optional columns to be present (when present) before mandatory columns?

@effigies
Copy link
Collaborator

@oesteban #1128 (comment):

Should be doable. Opened a validator issue for it. bids-standard/bids-validator#1948

The number of calibrations corresponding to this run.
type: integer
minimum: 0
CalibrationPosition:
Copy link
Collaborator

Choose a reason for hiding this comment

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

assumes the same position for all calibrations / validations

}
```

Content of `sub-01_task-VisualSearch_events.json`:

Choose a reason for hiding this comment

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

Is this really the _events.json file or is it the _physioevents.json?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is correct, it annotates the events.tsv file, as opposed to the new _physioevents.json.

Choose a reason for hiding this comment

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

So basically in the _physioevents.json file I only have the column names plus their description? And do I need a _events.json file if I do not have an _events.tsv? Because Martins dataset on Open Neuro does not have it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, gotcha -- you've actually hit an edge case we do not currently cover:

  • We say that some metadata belonging in the _events.json file becomes mandatory if eye tracking is present
  • However, BIDS allows task-rest to dismiss the events.tsv file. This BEP could make it mandatory for rest, even if the events.tsv file is just empty (only the header row).

WDYT @effigies @Remi-Gau ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Or include it in the physioevents.json? (if this does not disturb too much other stuff) Because that would be easy to include in the converter as we are already asking users to put some metadata manually in themetadata.yml which is read in by our code. The current code actually does that because I understood it wrong, cf my initial question...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure that is a good idea. If I were to move the metadata from events.json, I would think it'd be better in the physio.json file rather than physioevents.json.

These metadata are about stimulus presentation, so they are likely relevant beyond eye tracking exclusively. Perhaps a reasonable alternative would be to update the specs so these metadata are encoded within stim.json files instead of the events.json files. Either way, they need to allow a json be defined without the corresponding .tsv[.gz] file, or allow empty tables (okay for .tsv but dangerous for .tsv.gz).

Choose a reason for hiding this comment

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

Ok, I'm of course fine with the solution that fits best to BIDS in general but I agree that we should keep this information somehow.

allow a json be defined without the corresponding .tsv[.gz] file

would probably the best solution for this particular BEP as we agreed on .tsv.gz

```JSON
{
"DeviceSerialNumber": "17535483",
"Columns": ["timestamp", "x_coordinate", "y_coordinate", "pupil_size"],

Choose a reason for hiding this comment

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

In the src/schema/objects/columns.yaml and src/schema/rules/tabular_data/physio.yaml it says "et_timestamp", here it says "timestamp", which is correct?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both are correct, these are two different things:

  • src/schema/objects/columns.yaml, src/schema/rules/tabular_data/physio.yaml define a et_timestamp column name. This is so to allow the timestamp be different from the already existing timestamp column. This identifier is internal to the schema, so it does not propagate into actual column names.

  • the line 672 you commented on is just the necessary specification of the "Columns" metadata, the column name is arbitrary, and in this case it is named "timestamp" but other names would be equally supported.

src/modality-specific-files/physiological-recordings.md Outdated Show resolved Hide resolved
{
"SamplingFrequency": 100.0,
"StartTime": -22.345,
"Columns": ["cardiac", "timestamp"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe better if timestamp comes first, no?

selectors:
- suffix == "physioevents"
columns:
onset: required
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need new definitions for onset and duration because now the one used here assumed that the unit is in seconds.

src/modality-specific-files/physiological-recordings.md Outdated Show resolved Hide resolved
`sub-01_task-visualSearch_recording-eye1_physio.json` sidecar
could read:

```JSON
Copy link
Collaborator

Choose a reason for hiding this comment

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

insert in example here

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Aug 2, 2024

Discussed previously during some meetings with @oesteban @mszinte

Note that if the source data for single run was acquired by turning the eyetracker only during trials instead of keeping the eyetracking recording for the whole duration of the run, this will lead to discontinuous timestamps.

@julia-pfarr and I are encountering the issue in some of the datasets we are converting.

The decision for now is to pad the output files with rows of NaNs for the missing time points.
Note that this technically "creates" timepoints that were not recorded and that it may inflate the size of the output file.

Wonder if this consequence of using physio data for this kind of eyetracking acquisition should be mentioned somewhere in the spec, or if this is more a converter implementation detail + best practice recommendations for data acquistion...

@scott-huberty
Copy link

Hi everyone,

Note that if the source data for single run was acquired by turning the eyetracker only during trials instead of keeping the eyetracking recording for the whole duration of the run, this will lead to discontinuous timestamps.

Frustratingly, this is at least somewhat common in EyeLink eytrackers. Eyelink will also stop recording any time you enter a calibration sequence.

The decision for now is to pad the output files with rows of NaNs for the missing time points. Note that this technically "creates" timepoints that were not recorded and that it may inflate the size of the output file.

This is exactly what we did in the eyelink reader in MNE. I was also unsure if it was the right thing to do.. It makes me feel a little better about that decision, seeing you all arrive to the same conclusion independently 🙂

Wonder if this consequence of using physio data for this kind of eyetracking acquisition should be mentioned somewhere in the spec, or if this is more a converter implementation detail + best practice recommendations for data acquistion...

Just a heads up that our decision to pad with NaNs in MNE has caused its fair share of headaches, especially with signal processing routines (e.g. if you filter your pupil size signal, one of those NaN's could obliterate the whole signal!).

fields:
EnvironmentCoordinates: required
RecordedEye: required
SampleCoordinateUnits: required
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that the unit may also described in the description of each column of the physio file so so this may conflict with also having it here, no?

@qian-chu
Copy link

Hi everyone thanks for the hard work for pushing this forward! I have a question/minor suggestion: when timestamp is among the columns, shouldn't we also encourage users to provide metadata about it? For example the unit (ms for Eyelink, ns for Pupil Labs) and the reference frame (UNIX time or time since system startup). The current example doesn't provide such info:

```JSON
{
"DeviceSerialNumber": "17535483",
"Columns": ["timestamp", "x_coordinate", "y_coordinate", "pupil_size"],
"EnvironmentCoordinates": "top-left",
"Manufacturer": "SR-Research",
"ManufacturersModelName": "EYELINK II CL v4.56 Aug 18 2010",
"RecordedEye": "right",
"SampleCoordinateSystem": "gaze-on-screen",
"SampleCoordinateUnits": "pixel",
"SamplingFrequency": 1000,
"SoftwareVersion": "SREB1.10.1630 WIN32 LID:F2AE011 Mod:2017.04.21 15:19 CEST",
"ScreenAOIDefinition": [
"square",
[100, 150, 300, 350]
],
"pupil_size": {
"Description": "Pupil area of the recorded eye as calculated by the eye-tracker in arbitrary units (see EyeLink's documentation for conversion).",
"Units": "a.u."
}
}
```

type: object
items:
type: object
EyeTrackerDistance:

Choose a reason for hiding this comment

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

Instead of EyeTrackerDistance metadata object we can employ a much holistic EyetrackingGeometry object that records the spatial description of eyetracking setup. This is necessary information for correcting pupil foreshortening error during analysis.

It could look something like

"EyetrackingGeometry" : {
	"EyeToCameraX": 0.05,
	"EyeToCameraY": 0.30,
	"EyeToCameraZ": 0.40,
	"EyeToScreenTopLeftX": 0.15,
	"EyeToScreenTopLeftY": 0.1,
	"EyeToScreenTopLeftZ": 0.50
}

where X, Y are the axes that run along the width and height of the screen, while Z is the axis along which the subject, the eyetracker and the screen are located.

Copy link
Collaborator

@oesteban oesteban Sep 5, 2024

Choose a reason for hiding this comment

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

I would add these in addition to the simpler EyeTrackerDistance, noting that it is recommended to have these better metadata values. I'd suggest also:

{
  "EyeToCameraXYZ": [0.05, 0.30, 0.40],
  "EyeToScreenTopLeftXYZ": [0.15, 0.1, 0.5],
}

EncodingTechnique:
name: EncodingTechnique
display_name: Encoding Technique
description: |
The encoding technique used during readout.
For example, `"Cartesian"`, `"EPSI"`, `"Spiral"`,
or `"Density-weighted concentric ring trajectory"`.
type: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo due to merge conflict resolution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.