[FIX] Add OPTIONAL acq entity to channels.tsv, events.tsv to match electrophysiological acquisitions

[sappelhoff]

follow up on #675 that @gpiantoni had to drop due to time constraints.

[sappelhoff]

@tsalo I get the same issue as Gio upon calling python tools/bids_schema.py:

Traceback (most recent call last):
File "tools/bids_schema.py", line 467, in
_main()
File "tools/bids_schema.py", line 462, in _main
args.pop('func')
KeyError: 'func'

I think I have all the dependencies, so the error must be something else.

[tsalo]

It's a matter of usage, I believe. Here is how you need to call it to update the entity table:

bids-specification/tools/bids_schema.py

Lines 454 to 455 in f2545fe

	>>> python bids_schema.py entitytable ../src/schema/ \
	>>> ../src/99-appendices/04-entity-table.md

[sappelhoff]

indeed 🤦‍♂️ thank you!

[effigies]

Is this new title accurate? My understanding is that we're clarifying, not adding.

[sappelhoff]

Is this new title accurate? My understanding is that we're clarifying, not adding.

we are adding "acq" to "channels" for M(i)EEG, which wasn't present before, but which makes a lot of sense, if M(i)EEG JSONs can have that entity.

So it'd kind of ... a bugfix?

If you run a task (= 1 events.tsv) and record EEG with two amplifiers with potentially different settings (= 2 different *acq-<label>_eeg.<ext>), then the resulting channels files may HAVE to be different as well.

[robertoostenveld]

I think this is a clarification, not an extension. The channels.tsv needs to match filename-wise with the eeg data itself and the eeg.json. If <acq>-label is part of the eeg dataset file name, it should also be part of the channels.tsv. Idem for example for runs, in case runs would have different filter settings (and hence channels.tsv would be run-specific).

[sappelhoff]

I think this is a clarification, not an extension

to be clear: the questions whether this is a clarification or an extension is only important with regards to how we name the Pull Request.

Are you fine with the proposed changes other than the PR title, @robertoostenveld - or did you want to ask for additional changes?

[robertoostenveld]

I agree with all changes made in the PR, but wonder about a few that were not made. @gpiantoni might want to chime in

if two amplifiers were used, would there also be two events files? Since the start time of the data in the files is not identical, the event onsets in a single event file could only apply to one of them.

if two amplifiers were used, should there also be two electrode files? I mentioned this in https://neurostars.org/t/two-amplifiers-for-ieeg-recordings/17492/4, but technically I think that a single electrode file that contains both sets would suffice.

[sappelhoff]

agreed, an events file may be impacted as well. I have added that in the two most recent commits.

Electrode files already allowed for acq.

[sappelhoff]

@MaxvandenBoom @robertoostenveld @dorahermes could you please give this another look and an approving review if you agree?

[robertoostenveld]

looks good to me

[dorahermes]

lgtm