update eyetracker to eyetrack as BIDS data type in line with bep020

[henneysq]

This PR updates the data2bids to more closely resemble the behaviour intended by (BEP020)[https://docs.google.com/document/d/1eggzTCzSHG3AEKhtnEDbcdk-2avXN6I94X8aUPEBVsw/edit#heading=h.9tphvz6ot0j1].

Currently, the only changes made are:

1. suffix updated from 'eyetracker' to 'eyetrack'
2. 'eyetrack' dir added as destination for 'eyetrack' files

This PR does not (yet) address the specifications of sidecar content.

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_issue2235, test_data2bids, test_issue1196

When inside the DCCN, please also consider testing: test_issue1905, test_issue1196, test_data2bids, test_pull2043

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[robertoostenveld]

Please add prior to this a line

cfg = ft_checkconfig('renamedval', {'suffix', 'eyetracker', 'eye track'});

it can go before setting all defaults. This allows old scripts to keep working, and prints a warning. Otherwise people would probably get an error further down in the code.

[robertoostenveld]

please also add somewhere at the top

cfg = ft_checkconfig('renamed', {'eyetracker', 'eye track'});

to rename this field in case the user supplied it with the old name

[robertoostenveld]

you write that you changed it to

'eyetrack' dir added as destination for 'eyetrack' files

but that is not the case as far as I can see. On line 2589 it seems to go in the beh folder. In bids-standard/bids-specification#1128 I read

Eye-tracking data MUST be stored in the main data recording modality or
<datatype> directory (for example func when combined with fMRI, or
beh when combined with behavioral measures).

I believe this also applies to the remainder of {'events' 'stim' 'physio' 'audio' 'video'}. Should we add an option cfg.datatypedir which by default is empty and auto-determined using the datatype2dirname() function, but which allows it to be user-specified for eyetrack and such? There are now two calls to datatype2dirname(), those should then use the cfg.datatypedir field (which itself then needs to be set somewhere at line 725 or so).

[henneysq]

Hmm, I was referring to the google doc you sent, which shows the following convention:

sub-<participant_label>/
    eyetrack/
        sub-<participant_label>[_ses-<label>]_task-<task_label>[_acq-<label>]_eyetrack.tsv
        [sub-<participant_label>[_ses-<label>]_task-<task_label>[_acq-<label>]_eyetrack.json]
        [sub-<participant_label>[_ses-<label>]_task-<task_label>[_acq-<label>]_events.tsv]

Is that out of date? And should I refer to bids-standard/bids-specification#1128 instead?

As to the latter half: Yes, I think that's a good idea. I will implement that.

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_data2bids, test_issue1196, test_issue2235

When inside the DCCN, please also consider testing: test_pull2043, test_issue1905, test_data2bids, test_issue1196

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_issue1196, test_data2bids, test_issue2235

When inside the DCCN, please also consider testing: test_issue1196, test_issue1905, test_data2bids, test_pull2043

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[henneysq]

Perhaps, cfg.datatypedir should default to empty but be mandatory for {'events' 'stim' 'physio' 'audio' 'video' 'eyetrack'} to reflect the sentiment of the proposal:

"Eye-tracking data MUST be stored in the main data recording modality or <datatype> directory"

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_data2bids, test_issue2235, test_issue1196

When inside the DCCN, please also consider testing: test_pull2043, test_data2bids, test_issue1905, test_issue1196

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_issue1196, test_issue2235, test_data2bids

When inside the DCCN, please also consider testing: test_issue1905, test_issue1196, test_pull2043, test_data2bids

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_issue2235, test_issue1196, test_data2bids

When inside the DCCN, please also consider testing: test_issue1905, test_issue1196, test_pull2043, test_data2bids

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_issue2235, test_issue1196, test_data2bids

When inside the DCCN, please also consider testing: test_issue1905, test_pull2043, test_issue1196, test_data2bids

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[robertoostenveld]

... google doc ...
Is that out of date? And should I refer to bids-standard/bids-specification#1128 instead?

Yes, that is out of date. The discussion and development continued on the bep020 pull request, which is
bids-standard/bids-specification#1128. And that PR points to https://github.com/mszinte/bids-specification/tree/bep020 and that is where I think most recent developments have happened.

Perhaps, cfg.datatypedir should default to empty but be mandatory for {'events' 'stim' 'physio' 'audio' 'video' 'eyetrack'} to reflect the sentiment of the proposal:

"Eye-tracking data MUST be stored in the main data recording modality or directory"

good idea, it does not hurt to make it required for all of those.

[henneysq]

I realise today that when allocating eyetrack files to the functional imaging dir (e.g. meg), they compete for the sub-<participant_label>[_ses-<label>]_task-<task_label>[_acq-<label>]_events.tsv filename. So the latter of meg and eyetrack data to be converted overwrites the prior.

@robertoostenveld, does this imply that the two should be merged? In that process, one would need to ensure their allignment in time.

[henneysq]

Or is it in fact also an outdated requirement from the google doc that eyetrack should have an _events.tsv?

[github-actions]

You should test whether your modifications do not break anything.
See https://www.fieldtriptoolbox.org/development/testing/

When outside the DCCN, please consider testing: test_issue2235, test_data2bids, test_issue1196

When inside the DCCN, please also consider testing: test_issue1905, test_issue1196, test_data2bids, test_pull2043

Suggested tests outside the DCCN use public data or do not use data.
Suggested tests inside the DCCN use private data.

[henneysq]

In commit efb818e, I inverted the logic of the need_eyetrack_json when checking whether to write the _events.tsv. This resolves the issue, so that the MEG file gets to dictate the content of _events.tsv.

[robertoostenveld]

Or is it in fact also an outdated requirement from the google doc that eyetrack should have an _events.tsv?

Yes, I read in one of the comments on the commit that detected saccades (which are timestamped "events") are to be interpreted as derivative and not stored along with the raw data.

Do you have other events in your eye tracking recording, besides the saccades? Like triggers? In your "raw1" version you would still have the eyetrack.tsv and the corresponding events.tsv in the beh folder, right? I assume that, because in that version it is not yet aligned with the MEG.

In your "raw2" version you would align them along the same time axis and merge them, so events from the MEG and from the eyetracker could be fused in a single events.tsv (you would actually use those shared events for the alignment). So after time shifting also saccades from the raw1/beh/events.tsv could still be represented in the raw2/meg/events.tsv.

[henneysq]

Hmm I actually moved the eyetrack to meg already, but I probably should keep it in beh for raw1 as they are not alligned yet.

I have actually been working on how to figure out the allignment of eyetrack and meg. It appears that quite a bit of information is "lost" in the current implementation. I will try to explain it, but we should probably have a talk about it to make sure we understand eachother.

Before commit efb818e when eyetrack wrote an _events.tsv file, this contained FIX and SACC events, while _eyetrack.tsv contained the timeseries of x-/y-coordinates. If I understand it correctly, these are extracted from the source .asc file where the two are intermingled/stacked in time. However, after commit efb818e, the events are no longer saved anywere and get lost in the transition.

So what I will have to do is

1. In raw1, keep writing the _events.tsv file, but to beh
2. In raw2, align the time axes of meg and eyetrack and merge events into on _events.tsv file

To achieve 1., I will revert the change in commit efb818e and set datatypedir to beh.

I can achieve 2. locally, but I'm not sure I would be able to logically implement it into data2bids. Unless it can be implemented with a merge of the two that assumes that events are on the same timeline already. Let me know what you think.