[FIX] Clarify continuous recording metadata fields

[effigies]

This change brings the formatting of metadata associated with continuous recording files (_physio.tsv, _stim.tsv) in line with that of other metadata, namely in a tabular format with requirement level and units, hopefully making it easier to find with a quick glance.

Also, these files may no longer have start times purely in relation to volumetric time series like BOLD. Do we need to update the language so these can be used in relation to EEG/MEG/iEEG? Or do those modalities use different formats for physio data?

[emdupre]

This LGTM, but I'm not sure on your question about non-MRI modalities... Pinging @jasmainak, @choldgraf, @dorahermes for opinions !

[jasmainak]

I'm seeing this part of the spec for the first time :) I think @sappelhoff may have better opinions

In MEG, audio stimuli and such are often stored as MISC channel in the same file. So it figures in the _channels.tsv with the channel type and sampling frequency set appropriately.

[dorahermes]

This sounds like it only applies to MRI data

[choldgraf]

Agree- what about "Start time in seconds in relation to the first data sample in the corresponding neural dataset (negative values are allowed)"

[effigies]

I'm assuming "start of acquisition of the first volume" is an attempt to precisely define the onset of the first data sample. What about: "Start time in seconds in relation to the start of acquisition of the first data sample in the corresponding neural dataset (negative values are allowed)."

[effigies]

Hmm. I'm noticing that this appears in func/. Is that going to cause problems with MEG/EEG/iEEG? Or would this be similar to a simultaneous fMRI/EEG?

[sappelhoff]

What do you mean by

I'm noticing that this appears in func/

?

+1 on changing to: "Start time in seconds in relation to the start of acquisition of the first data sample in the corresponding neural dataset (negative values are allowed)." as @effigies suggests

[effigies]

@sappelhoff I mean that on line 7, we specify that these files occur in the func/ subdirectory. Should this be expanded to include other directory names?

[sappelhoff]

Ah I see, yes it could be made more general, for example with a <modality> placeholder? The text below would have to be adjusted so that the <matches> placeholder does not only refer to MRI data 👍

[dorahermes]

it is probably good to add units as optional

[sappelhoff]

I agree

[effigies]

I was moving to add this, but we should probably consider how to make this as conformant as possible with https://bids-specification.readthedocs.io/en/stable/02-common-principles.html#tabular-files.

For example, we could then allow the same metadata for each column (LongName, Description, Levels, Units, TermURL). So something like:

{
  "SamplingFrequency": 10,
  "StartTime": 0.0,
  "Columns": ["cardiac"],
  "cardiac": {
    "LongName": "continuous pulse measurement",
    "Units": "mV",
    "TermURL": "..."
  }
}

[sappelhoff]

+1, looks like a good reuse of existing structure

[sappelhoff]

Agreeing to two already proposed but not yet implemented changes and having one open question (see review comments)

[sappelhoff]

What do you mean by

I'm noticing that this appears in func/

?

+1 on changing to: "Start time in seconds in relation to the start of acquisition of the first data sample in the corresponding neural dataset (negative values are allowed)." as @effigies suggests

[sappelhoff]

I agree

[effigies]

I think GitHub got confused and decided that there were merge conflicts where there weren't. I merged master and now the conversations are detached from the source.

@sappelhoff Sorry for the back-and-forth, but I want to push off the EEG/MEG/iEEG fixes for another PR, as I think it applies to Task events and Physiological and other continuous recordings, and deserves close attention from the EEG/MEG/iEEG crowd. They are more likely to notice duplications from their sections that can be cleaned up, or contradictions that need to be resolved. I think it would be pushing too much on this PR which is mostly just a language clarification.

I'm happy to include discussion of units and related metadata, but it is changing the structure of the file, so I'm going to rope in @tyarkoni as the PyBIDS implementor-in-chief to see if there are issues we hadn't considered.

I believe what I've proposed in #167 (comment) is consistent with the derivative Time series metadata. It would be nice to get a second opinion on that.

[sappelhoff]

LGTM and fine to have the EEG/MEN/iEEG changes in another PR.

[emdupre]

This LGTM ! Thanks, @effigies !