[FIX] Re-add run entity to electrodes.tsv

[effigies]

In BIDS 1.3, EEG had

sub-<label>[_ses-<label>][_acq-<label>][_run-<index>]_electrodes.tsv

and iEEG had

sub-<label>[_ses-<label>][_space-<label>]_electrodes.tsv

As of 1.5, presumably due to schema stuff, they both became

sub-<label>[_ses-<label>][_acq-<label>][_space-<label>]_electrodes.tsv

So EEG lost run and gained space, and iEEG gained acq. Looking in OpenNeuro, there are iEEG electrodes files with acq and run and EEG with space. So we should just permit all three for both, as trying to bottle this genie isn't worth the pain.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.93%. Comparing base (c9e4779) to head (e7154a3).
Report is 30 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1722   +/-   ##
=======================================
  Coverage   87.93%   87.93%           
=======================================
  Files          16       16           
  Lines        1351     1351           
=======================================
  Hits         1188     1188           
  Misses        163      163           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[sappelhoff]

for EEG at least, changing "electrodes" within a run would typically mean a new session -- so, a "run" entity for electrodes in EEG doesn't make sense. I don't remember what happened between the versions unfortunately, and it's unfortunate that there are datasets that use the run entity on EEG electrodes 🤔

cc @robertoostenveld @dorahermes

[dorahermes]

We typically assumed when we wrote the spec that changing electrode positions would result in a different session.

Is there perhaps an example for acquisition in iEEG electrodes? I can imagine adding acquisition on the channels as that can pertain to the sampling frequency etc, but not sure what the use case would be for electrodes.

[effigies]

I can dig up the data next time I'm at my machine, but the point of this is not that there's a good use case but that the validator has permitted it since the BEPs were merged and this flexibility has led to people using it.

I suspect in many cases they are just copied to match the data files, like sidecars with no inheritance, not actually varying.

[dorahermes]

Thinking about it a bit more acquisition for iEEG could potentially indicate CT/MRI/Photo, moving forward I think this is ok.

I am not sure if run makes sense without an optional task label etc. Not sure how packages deal with this @robertoostenveld

[Remi-Gau]

for EEG at least, changing "electrodes" within a run would typically mean a new session -- so, a "run" entity for electrodes in EEG doesn't make sense. I don't remember what happened between the versions unfortunately, and it's unfortunate that there are datasets that use the run entity on EEG electrodes 🤔

cc @robertoostenveld @dorahermes

Is this related to this?

#1481

[robertoostenveld]

I can imagine that people copy the full filename of the EEG file, and only replace the _eeg suffix with _electrode, consequently the electrode filename would also contain the task entity. I could even imagine that happening when people use automatic converters, like EEGLAB or a FieldTrip script.

There is no real point in the task entity (nor in the run entity) for the electrodes, but I also don't see much harm as long as there is an unambiguous mapping between the EEG file and the electrode positions. Here I am thinking of the perspective of a dataset downloader/user: having superfluous entities in the electrode file name does not interfere with that mapping.

I would in general recommend not to use entities that are not needed. For example, with a single EEG recording, I would not use run-1. And with digitized EEG electrode positions, the run and task entity are not needed and hence I would not use them either. My rule would be: keep filenames as short as possible, and as long as needed.

Note that for iEEG I believe that we did agree on the acq entity to be used, and also for EEG I believe that makes sense in some cases (like with a pre and post digitization of EEG electrode positions). We are now working on OPM MEG coregistration, and also there I can imagine that versions of the OPM sensor locations can be specified. Note however that OPM locations stored separately from the data file at the moment falls outside of the spec, but there is some work to be done on BIDS to accomodate OPMs (as indicated by the suggested OPM BEP).

[effigies]

I suggest we add the following text to each electrodes.tsv section:

*_electrodes.tsv files SHOULD NOT be duplicated for each data file,
for example, during multiple runs of a task.
The inheritance principle MUST
be used to find the appropriate electrode positions for a given data file.
If electrodes are repositioned, it is RECOMMENDED to use multiple sessions to indicate this.

This would allow the existing duplication without invalidating datasets, direct the validator to raise a warning when detecting this case (I'll write a schema rule), and indicate clearly to tooling to search for the relevant file rather than hard-code assumptions about what entities should be there.

Would this PR be acceptable if I do the above?

[effigies]

@dorahermes @robertoostenveld Could I bother either of you for a review?

[robertoostenveld]

Looks good to me.

[effigies]

Thanks!