[FIX] add optional _acq-<acquisition> to meg, eeg, ieeg, channels#675
[FIX] add optional _acq-<acquisition> to meg, eeg, ieeg, channels#675gpiantoni wants to merge 3 commits intobids-standard:masterfrom gpiantoni:master
Conversation
sappelhoff
left a comment
There was a problem hiding this comment.
indeed, and #610 would fix that as well. But as #610 will probably be merged only in some time, I think merging this PR ahead of #610 is sensible.
A new change is to add acq as optional for CHANNELS files. That makes sense to me, because if some data is recorded with a different ACQ, it may affect things such as reference, sampling frequency, etc, which can be written into CHANNELS.
@gpiantoni could you please add an optional ACQ to eeg and meg CHANNELS templates as well? As far as I can see you only did that for iEEG currently.
|
Thank you for the review. Sorry, I added the additional field to |
|
cc @bids-standard/raw-electrophys - please review |
|
Can you add bids-specification/src/schema/auxdatatypes/channels.yaml Lines 11 to 15 in 744b3d4 Then run |
tsalo
left a comment
There was a problem hiding this comment.
Thank you for this change. Two notes- first, the options for the value in an entity key-value pair are "index" and "label". The appropriate one for acq is label. Second, once you've updated the schema file, please use bids_schema.py to update the entity table, rather than editing the entity table file directly.
src/99-appendices/04-entity-table.md
Outdated
| | meg<br>(headshape) | REQUIRED | OPTIONAL | | OPTIONAL | | | OPTIONAL | | | ||
| | meg<br>(markers) | REQUIRED | OPTIONAL | OPTIONAL | OPTIONAL | | | OPTIONAL | | | ||
| | channels<br>(meg eeg ieeg) | REQUIRED | OPTIONAL | REQUIRED | | OPTIONAL | | | | | ||
| | channels<br>(meg eeg ieeg) | REQUIRED | OPTIONAL | REQUIRED | OPTIONAL | OPTIONAL | | | | |
There was a problem hiding this comment.
The entity table shouldn't be edited directly. It should be generated from the bids_schema.py script.
There was a problem hiding this comment.
OK, sorry about that. I restored the original 99-entity-table.md.
|
Sorry about editing the entity table. Unfortunately, I get an error when running
Unfortunately I don't have time into debugging this issue. |
|
No worries @gpiantoni we will take it from here. Thanks for starting on the PR. |
|
PS: @gpiantoni if you want to add your name to the BIDS contributors, you are welcome to do so. Just edit this Wiki page: https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors we'll make sure your name is added to the appendix before the next release of BIDS. |
|
I did not see @robertoostenveld comment on this. I am personally agnostic to the addition of this optional file to the EEG, MEG, iEEG BIDS format. For EEG though, there is a JSON file with already a lot of information about data acquisition. |
|
Hi Arno, this was discussed (also by me) on https://neurostars.org/t/two-amplifiers-for-ieeg-recordings/17492. The PR follows up on that discussion. Rather than only adding |
This is not only for UPDATE, oh this is already in https://github.com/bids-standard/bids-specification/blob/master/src/schema/datatypes/eeg.yaml and the discussion actually should better continue in the PR #677 |
Following up on the discussion in Neurostars,
acq-<acquisitionis an optional field formeg,eeg,ieeg,channelsaccording to the Entity Table. This fix updates the individual pages related tomeg,eeg, andieeg.