-
Notifications
You must be signed in to change notification settings - Fork 163
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
base: master
Are you sure you want to change the base?
Conversation
correction of text
(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. |
|
The number of calibrations corresponding to this run. | ||
type: integer | ||
minimum: 0 | ||
CalibrationPosition: |
There was a problem hiding this comment.
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`: |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 theevents.tsv
file is just empty (only the header row).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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"], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 aet_timestamp
column name. This is so to allow the timestamp be different from the already existingtimestamp
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.
selectors: | ||
- suffix == "physioevents" | ||
columns: | ||
onset: required |
There was a problem hiding this comment.
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.
`sub-01_task-visualSearch_recording-eye1_physio.json` sidecar | ||
could read: | ||
|
||
```JSON |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
insert in example here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What example is missing here?
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. 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... |
Hi everyone,
Frustratingly, this is at least somewhat common in EyeLink eytrackers. Eyelink will also stop recording any time you enter a calibration sequence.
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 🙂
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 |
There was a problem hiding this comment.
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?
Hi everyone thanks for the hard work for pushing this forward! I have a question/minor suggestion: when bids-specification/src/modality-specific-files/physiological-recordings.md Lines 670 to 691 in 26c463a
|
for more information, see https://pre-commit.ci
src/schema/objects/metadata.yaml
Outdated
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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inputs from previous meeting for inserting details about Eyetracking setup 'geometry'
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to me, thanks
We are eager to release our HBN dataset (2000+ subjects with eye tracking). I have provided several candidates on another thread matching the proposed format. When can we expect the format finalized? Tx |
To my opinion the BEP is ready, we are finishing examples and testing deeply our converter. I would then suggest you to take it as it is and keep an eye on this thread in case some reviewer makes important changes when we will open it to review. Best, |
name: pupil_size | ||
display_name: Pupil size | ||
description: | | ||
Pupil size or area of the recorded eye, in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we recording the particular type of pupil size that's being recorded here ('diameter' v/s 'area')?
The data that comes out of Eyelink is always in 'arbitrary units' (Eyelink documentation: page 106, 4.4.3) and the actual data type is stored in the settings (Eyelink documentation: page 25 'Pupil Size Data').
Here is the specifications of the BEP 020 about eye tracking.
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
Tip
HTML preview of this BEP
Issues for: