fixes to channels.tsv and electrodes.tsv in some datasets?

[drammock]

draft; needs #2 rebased onto emg_examples after #2 was merged

@neuromechanist while working on #2 I noticed some odd things about channels.tsv and electrodes.tsv in some datasets. This PR hopefully helps show what I think was odd. but when you view the files diff you'll need to view only the most recent commit ("fixup channels & electrodes files in TwoWristbands & dataset"). Here are the issues I saw and tried to fix here:

1. it's supposed to be allowed for electrode name to be non-unique, as long as the name+group combination is unique. So first off, I changed the electrode names in grid2 to restart numbering at zero (so we can test that the validator handles that case properly).

2. I also had to rename them in the signal_electrode column in channels.tsv, which made me realize that the group column in the channels file is really meant to mean "group that signal_electrode belongs to" (not "group that this channel belongs to). As far as I can tell, there shouldn't ever be a case where channels have the same name but different groups, right? In other words, we only need a group column in channels.tsv in cases where signal_electrode is also specified.

   If you agree, I'll make a slight tweak to the definition of that column in the spec, and change the file rule so that only name (not name + group) must be unique for channels. (for electrodes, the rule should remain as-is).

3. while looking at channels.tsv, I noticed a placement column that recapitulated the xyz info in electrodes.tsv. I removed that, because it's redundant with the info in electrodes.tsv; putting measured locations in channels.tsv should only be done if electrodes.tsv is absent I think.

4. That last point also made me realize that (a) the correct column name is placement_scheme, not placement, and (b) placement_scheme must be one of "measured" or "other" (if "other", details go in placement_scheme_description). So I fixed that issue in the IndependentMod dataset's channels.tsv file (it had the description content in the placement_scheme column) and updated its main emg.json correspondingly.

Points 1 and 2 above I think also apply to the concurrentIndependentUnits dataset, but I haven't changed those here yet; want to hear your thoughts on all the above first.

[neuromechanist]

I think the only blocking is placement_description. Otherwise, all good

[neuromechanist]

In the spec, this is now placement_description. Should we change?

[drammock]

oops, yeah

[neuromechanist]

it's supposed to be allowed for electrode name to be non-unique, as long as the name+group combination is unique. So first off, I changed the electrode names in grid2 to restart numbering at zero (so we can test that the validator handles that case properly).

This makes sense to me.

I also had to rename them in the signal_electrode column in channels.tsv, which made me realize that the group column in the channels file is really meant to mean "group that signal_electrode belongs to" (not "group that this channel belongs to). As far as I can tell, there shouldn't ever be a case where channels have the same name but different groups, right? In other words, we only need a group column in channels.tsv in cases where signal_electrode is also specified.

I think for the case of the wristband, your reasoning would not hold actually 😅. We are using electrodes.tsv to hack our way for channel placement when electrode information is not available. So, for this example, it is the other way around; the group in this TwoWristBand example means the channels belonging to the group, and we don't have information about he electrodes from the data explicitly. (This will happen with, now emerging, consumer EMG products. Your determination is spot on for the twoHDsEMG example).

Your point on channels should not have the same names but different groups seems correct, as channels are tied to the data, and you can't have two channels with the same name.

[drammock]

I think for the case of the wristband, your reasoning would not hold actually 😅. We are using electrodes.tsv to hack our way for channel placement when electrode information is not available. So, for this example, it is the other way around; the group in this TwoWristBand example means the channels belonging to the group, and we don't have information about he electrodes from the data explicitly. (This will happen with, now emerging, consumer EMG products. Your determination is spot on for the twoHDsEMG example).

Your point on channels should not have the same names but different groups seems correct, as channels are tied to the data, and you can't have two channels with the same name.

I think I agree with you from a "what is the reality of the hardware" point of view. So my edit, where I changed electrodes.tsv to re-set naming at the beginning of group 2 --- that shouldn't happen in this dataset (but as you say, it should occur in the TwoHDsEMG dataset). But I think that also means that in TwoWristbands dataset we shouldn't need a group column at all, either in channels.tsv or in electrodes.tsv. Of course the researcher can include a column with values left-band and right-band if they want to, and they could call it "group" if they want to I guess.

But from a general "BIDS dataset" point of view of the TwoWristbands case: either the two bands are recording into the same data file, or 2 different data files. If it's the same file, they MUST have distinct channel names (as they currently do). If it's 2 different data files, the names need not be unique because there would be distinct channels.tsv for each data file (distinguished by recording entity). So I think what that means is that you should never need a group column in channels.tsv UNLESS you also have a signal_electrode column and the signal_electrode names are non-unique. And if that's correct, then I think we need to change the spec so that the channels.tsv uniqueness-of-rows condition is based only on [name] not on [name, group].

[neuromechanist]

But I think that also means that in TwoWristbands dataset we shouldn't need a group column at all.

Well, yes, but exactly as you mentioned, if I were the data curator, I would add it, so the reader would understand that from Ch16 onward is another hand.

I think we need to change the spec so that the channels.tsv uniqueness-of-rows condition is based only on [name] not on [name, group]

Yes. Agreed

[drammock]

ugh, it looks like you've been pushing fixes to emg_examples branch, so there's a merge conflict here now. Are you going to push any more? I don't want to try to resolve merge conflicts on a moving target

[drammock]

ugh, it looks like you've been pushing fixes to emg_examples branch, so there's a merge conflict here now. Are you going to push any more? I don't want to try to resolve merge conflicts on a moving target

never mind, I think I sorted it out

[drammock]

OK @neuromechanist I think this is ready for merge