@sappelhoff It looks like the validator actually supports acq, rec, and run already (see here). Plus the physio and stim suffixes under beh allow recording as well (see here). So it looks like the specification is a ways behind the validator on this modality, at least.

Should I open an issue or update this PR to include those other entities for beh data?

I don't think we need to document recording in BEH, because that entity is "standalone" for physio+stim files and documented in the relevant section.

That leaves acq and rec. Do they even make sense for BEH recordings?

I don't see any reason to support rec or acq for behavioral data based on the specification, which makes me wonder why the validator supports both.

Still, perhaps this mismatch between the specification and the validator is something that can wait to be resolved by the transition to a schema.

which makes me wonder why the validator supports both.

copy pasting without reading the spec fully :-) ... it can happen, because some of the validator devs are not specification users. (and everybody makes mistakes) --> But if we find such things it'd be good to correct it.

Still, perhaps this mismatch between the specification and the validator is something that can wait to be resolved by the transition to a schema.

I agree unless the fix is easy, in which case we should act as soon as somebody can find the time.

for this PR: I think we can thus leave it as is:

• recording is documented in physio+stim
• acq and rec need to be fixed in validator (make them go away for beh)
• merge this PR to introduce run for beh data
  • validator already covers it

I've opened https://github.com/bids-standard/bids-validator/issues/1036, so at least it's on their radar. Should we then merge this PR?

I've opened bids-standard/bids-validator#1036, so at least it's on their radar. Should we then merge this PR?

we have a loose rule to leave PRs open for 5 days after two approving reviews, so that other people have the chance to chime in if they want to.

Let's leave it open for a bit longer and then merge if nobody complains :-)

Per the maintainers call today, we want to support acq as well. I think I also need to update the entity table to include recording support for beh/*_physio and beh/*_stim, which will mean splitting that row.

I also added beh as a suffix under the beh data type in the entity table.

Looks good. Can we add an example acq-DBS/acq-noDBS to demonstrate the usage?

Absolutely. Would something like the following be good?

The OPTIONAL acq-<label> key/value pair corresponds to a custom label one may use to distinguish different set of parameters used for acquiring the same task. For example this should be used when a study includes runs of an n-back task - one with deep brain stimulation and one without. In such a case, the sub-01_task-nback_acq-dbson_beh.tsv and sub-01_task-nback_acq-dbsoff_beh.tsv, however the user is MAY choose any other label than dbson and dbsoff as long as they are consistent across subjects and sessions and consist only of the legal label characters.

This is just cribbed from the corresponding paragraph under Task (including resting state) imaging data and changed with the DBS example.

I would keep it a little simpler:

The OPTIONAL acq-<label> key/value pair corresponds to a custom label to distinguish different conditions present during multiple runs of the same task. For example, if a study includes runs of an n-back task, with deep brain stimulation turned on or off, the data files may be labelled sub-01_task-nback_acq-dbson_beh.tsv and sub-01_task-nback_acq-dbsoff_beh.tsv.

Does that seem clear, or does it feel like I've removed too much?

I think that's pretty clear. I'll add it. Thanks!

We want to wait five days from the last commit, right? So, barring additional concerns, this should be mergeable on Monday?

Yeah, I think Monday would be reasonable. There's been no negative comment at all, so I don't think we need to be too conservative on a pretty minor change that ratifies existing datasets.

Thanks @tsalo