mne-bids icon indicating copy to clipboard operation
mne-bids copied to clipboard

channels.tsv on ds000117

Open agramfort opened this issue 2 years ago • 17 comments

the ds000117 contains a channels.tsv file but it's one file for all runs of a subject and it's not in the meg folder:

https://openneuro.org/datasets/ds000117/versions/1.0.5/file-display/sub-01:ses-meg:sub-01_ses-meg_task-facerecognition_channels.tsv

as the consequence read_raw_bids does not find the file and therefore cannot read the bad channels or the fact the EEG061 and EEG062 are EOG and EEG063 is ECG.

I ran the validator on the dataset and it does not complain about this.

@hoechenberger @sappelhoff @alexrockhill is it a bug of the dataset and validator or mne-bids?

presently I am stuck...

agramfort avatar Apr 25 '22 16:04 agramfort

I think probably mne-bids should use that channels sidecar and it's a bug/not-yet-implemented on the mne-bids side. I didn't write that part so I'd have to look into it a bit more but AFAIK the channels does not use hierarchy and just looks for a file at the data level.

alexrockhill avatar Apr 25 '22 17:04 alexrockhill

From the code, it looks like it should work. If you do mne_bids.path._find_matching_sidecar(bids_path, suffix='channels', extension='.tsv') do you get a the file you linked?

alexrockhill avatar Apr 25 '22 17:04 alexrockhill

the search str used is:

".../ds000117/sub-01/**/meg/sub-01_ses-meg*channels.tsv"

so it looks inside "meg" and not in there. It's one level up...

agramfort avatar Apr 25 '22 19:04 agramfort

I ended up moving the files to the meg folder for now...

agramfort avatar Apr 25 '22 19:04 agramfort

Maybe mne-bids should search without datatype if datatype doesn't return a match. And then maybe without subject even...

alexrockhill avatar Apr 25 '22 19:04 alexrockhill

I think it's a bug in the dataset, see also https://github.com/bids-standard/bids-examples/issues/296

sappelhoff avatar Apr 25 '22 20:04 sappelhoff

Do you think there would be harm in being more permissive with mne-bids and applying the hierarchical structure even if it's not supported by BIDS? As I've seen, not all datasets fit cleanly into BIDS yet and sometimes this kind of thing is helpful to being maximally flexible as long as the lowest level match is used first.

alexrockhill avatar Apr 26 '22 17:04 alexrockhill

Do you think there would be harm in being more permissive with mne-bids and applying the hierarchical structure even if it's not supported by BIDS?

We don't even really apply the hierarchical structure for BIDS-compliant datasets, and I'm one of those who believe that the hierarchical inheritance principle was a big mistake and should be killed in a future BIDS version 😅 But if you want to give its implementation a shot – sure, and good luck 🥸

hoechenberger avatar Apr 26 '22 20:04 hoechenberger

Also not a fan of hierarchical everything. Makes implementation a complex hell.

adam2392 avatar Apr 26 '22 20:04 adam2392

Haha, well maybe I'll try a PR if I have a bit, I think it can be elegant. Maybe more opinions will come to the defense of hierarchical data structures. They sure do like it in BIDS.

alexrockhill avatar Apr 26 '22 20:04 alexrockhill

if you want a deep read on suggested changes to the inheritance principle, you can also see this: https://github.com/bids-standard/bids-specification/pull/1003 and comment with your opinion :)

I think I'd be okay with an implementation of the inheritance principle in mne-bids ... but I would be against implementing support for stuff that's not allowed in BIDS

sappelhoff avatar Apr 26 '22 20:04 sappelhoff

I agree but I think an important caveat is that if it passes the BIDS-validator and has for some time, even if it's not part of the BIDS-specification, I think that should be supported for backward compatibility for as long as possible.

alexrockhill avatar Apr 26 '22 21:04 alexrockhill

inheritance principle is necessary for folks who manually curate the datasets since sometimes it's easier to edit one file than multiple files. For jsons you'd have to get a list of dictionaries across levels and merge them (with lower levels taking precedence). I don't know how it would work for tsv though :)

jasmainak avatar Apr 27 '22 03:04 jasmainak

~~Afaik, inheritance is not implemented for tsvs~~ (it's not implemented for "data" tsvs ... but it IS implemented for metadata tsvs ... it'd difficult when one would also encode metadata in a data tsv file -- which is possible due to being able to add arbitrary columns to any tsv)

sappelhoff avatar Apr 27 '22 06:04 sappelhoff

I agree but I think an important caveat is that if it passes the BIDS-validator and has for some time, even if it's not part of the BIDS-specification, I think that should be supported for backward compatibility for as long as possible.

In fact, we do support some things that are not in compliance with (the most recent revisions of) the specs. So, yeah, I'm okay with being lenient in certain cases, but we'll have to be careful not to allow for too much sloppiness :)

hoechenberger avatar Apr 27 '22 08:04 hoechenberger

Afaik, inheritance is not implemented for tsvs

Yes, this is a tricky case because some tsvs are data (i.e. _beh.tsv) and some are sidecars (e.g. _channels.tsv). I think the sidecar designation is more important than the file extension in determining inheritance and IMO inheriting sidecar files that are repeated across subjects/sessions/runs is much more DRY and therefore preferable.

alexrockhill avatar Apr 29 '22 23:04 alexrockhill

please also see: https://groups.google.com/g/bids-discussion/c/QmwChe4IREY

sappelhoff avatar May 14 '22 08:05 sappelhoff