[FIX] split MEG files should be listed separately in scans.tsv

[eort]

As discussed in #692 in the mne-bids repo, MEG recordings that consist of split files, all parts should be listed separately in the scans.tsv file.

[sappelhoff]

LGTM, thanks @eort

[sappelhoff]

@eort could you please add yourself to the contributors list here --> https://github.com/bids-standard/bids-specification/wiki/Recent-Contributors

[ftadel]

I haven't read the context, but it should be clear that is only about FIF files.
For other CTF and 4D MEG recordings, the split files are within the folder referenced in the scans.tsv.
(for other manufacturers, I don't know...)

Note that the split FIF files are linked with hard-coded file names (tag FIFF.FIFF_REF_FILE_NAME).
Ideally, renaming them should be done carefully by a dedicated program.
Does MNE-BIDS does this?

[ftadel]

SHOULD or MUST:
preferably MUST, the least ambiguity the better

[hoechenberger]

Ideally, renaming them should be done carefully by a dedicated program.
Does MNE-BIDS does this?

I'm actually not entirely sure how well this is currently being handled, but if we're not doing this properly in MNE-BIDS, then it's a bug we need to fix. :)

[eort]

Ideally, renaming them should be done carefully by a dedicated program.
Does MNE-BIDS does this?

I can't say anything about how consistently MNE-BIDS does that, but for my data it works fine.

[sappelhoff]

I'm actually not entirely sure how well this is currently being handled, but if we're not doing this properly in MNE-BIDS, then it's a bug we need to fix. :)

pretty sure we are just using MNE-Python for that in mne-bids, and MNE-Python does properly rename multi-part FIF files. It's even mentioned as an example/recommended tool to use in the bids spec

several open points to discuss as revealed in maintainers meeting

[effigies]

We should clarify:

1. Multi-part runs should be discussed in scans.tsv, not just in split, and the same acq_time is REQUIRED across parts of the same run (e.g., echo-, part-, split-)
2. Explicit note in split that acq_time should be the same for all splits, not acq_time(split2) == acq_time(split1) + duration(split1)
3. scans.tsv remains OPTIONAL

[tsalo]

1. Multi-part runs should be discussed in scans.tsv, not just in split, and the same acq_time is REQUIRED across parts of the same run (e.g., echo-, part-, split-)

My personal opinion is that the idea that all files should be listed in scan.tsv (when provided), should specifically be brought up in the description for scans.tsv and not in the definition for split.

Something like:

When the OPTIONAL scans.tsv file is provided, split files MUST have the same acquisition time (acq_time).

EDIT: We can open a separate issue about the definition of scans.tsv if you don't want to cover it in this PR.
I've opened bids-standard/bids-validator#1192 about associated validation.

[robertoostenveld]

@tsalo wrote

...all files should be listed in scan.tsv (when provided), should specifically be brought up in the description for scans.tsv ...

For FieldTrip data2bids and examples I have also been pondering about what is to be listed in the scans.tsv file. That is most explicit here. But I don't find it so clear what constitutes data (to be listed) and what metadata (not to be listed).

E.g. for PET there are the pet.nii files (obviously), but should the blood.tsv files also be listed in the scans.tsv? And for fMRI the bold.nii files would be listed in case scans.tsv is provided, but should physio, stim and events also be listed in that case?

However, for the concrete case here (split/part/echo) I don't think this confusion applies. I do like the suggestion to require acq_time to be the same for all.

[eort]

all files should be listed in scan.tsv (when provided), should specifically be brought up in the description for scans.tsv and not in the definition for split

Yes, I agree. But should these two things be only mentioned in the scans.tsv section or also in the split sections?

[sappelhoff]

E.g. for PET there are the pet.nii files (obviously), but should the blood.tsv files also be listed in the scans.tsv? And for fMRI the bold.nii files would be listed in case scans.tsv is provided, but should physio, stim and events also be listed in that case?

That's a good point that we should clarify as part of a bigger issue/PR 👍

Multi-part runs should be discussed in scans.tsv, not just in split, and the same acq_time is REQUIRED across parts of the same run (e.g., echo-, part-, split-)

This should be specified in this section: https://github.com/bids-standard/bids-specification/blame/master/src/03-modality-agnostic-files.md#L326

@eort can you make a proposal please?

[sappelhoff]

cool, I think this reads well now and addresses all initial criticisms and suggestions

[eort]

Nice!

Once this is merged, does the validator need to be updated to test for that change?

[sappelhoff]

Once this is merged, does the validator need to be updated to test for that change?

Taylor has opened an issue for that: bids-standard/bids-validator#1192

but it will be non-trivial to implement, unless you are well versed in JS :) (in the latter case it may or may not be easy)

[tsalo]

Thanks for the hard work on this. There is one minor typo, and I have a thought about wording. I'd prefer to avoid the term "split file," since it makes it harder to distinguish grouping elements (such as "run", "acquisition", and "file"). While a "run" or "acquisition" can correspond to multiple files, I think the singular word "file" should only apply to one actual file object.

[eort]

@tsalo Makes perfect sense! I adopted your suggestions.

[eort]

According to ci/circlci, there seems to be a certificate issue for https://www.cognitiveatlas.org/task/id/trm_54e69c642d89b
I don't know what to do about it.

[sappelhoff]

According to ci/circlci, there seems to be a certificate issue for https://www.cognitiveatlas.org/task/id/trm_54e69c642d89b
I don't know what to do about it.

that's nothing we can solve - we'll ignore it for now and revisit if it doesn't fix itself within a couple of weeks.

[rwblair]

Sorry about that, cert should be renewed now.

[effigies]

Restarted the Circle job.

[tsalo]

LGTM. Thanks!

[sappelhoff]

Thanks @eort