[ENH] BEP032: Resolve remaining documentation TODOs - #2504
Conversation
Remove the references to Setup and Procedure nodes in the sidecar JSON section, since the schema defines a flat sidecar. Replace the events.tsv TODO with a description of how the standard columns mark recordings in a multi-recording file. Point the example dataset section at the bids-examples datasets instead of the stale GIN repositories. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjq3vyBQs3SGtfrPRvp8Ws
yarikoptic
left a comment
There was a problem hiding this comment.
THANK YOU for pushing this forward! Overall -- good (although my claude might say smth too lateR). I feel we better drop and reiterate on that section on _events.tsv!
| onset duration trial_type | ||
| 0.0 120.0 baseline | ||
| 125.5 300.0 current-steps | ||
| 430.0 180.0 current-steps |
There was a problem hiding this comment.
I am confused here -- two of current-steps? then it is not a unique label to tell those recordings apart.
Overall concern:
But also, might be unrelated to limited scope/intent of this PR and I had missed this in prior discussions/meetings. I am confused about this overall section, as AFAIK _events.tsv SHOULD primarily be about stimuli or behavior events during acquisition. I guess we could come up with some scheme here to label offsets for different recordings within the file, and potentially via trial_type or extra columns (like originally envisioned seems to me) but to me it feels too ad-hoc/fragile/confusing! I feel it better needs some form of _recordings.{tsv,json} alike to a join of _scans.tsv and _electrodes.tsv, not abuse of _events.tsv. WDYT?
As such, this "clarification" just brought more questions than gave me answers.
There was a problem hiding this comment.
current-steps is a trial type, and trial types should typically not be unique, but reoccurring. it's just one event concept that starts and ends at some point, and can reoccur (or could be unique, depending on the experimental design)
There was a problem hiding this comment.
here we go -- so we have events not "recordings" as modified text above suggests... even if we make it work as it could be such regular repeating events and then some labels to tell offset for "recordings" -- it would be too confusing IMHO and does warrant a separate file!
There was a problem hiding this comment.
Decided to push the recordings/scans refactor for another PR long after BEP032
For now, just rephrase the example to not use 'trial_type' and be more device-centric and reflect the nature of the 'device being activated'
There was a problem hiding this comment.
(and add some rows to indicate irregularity - or as close as we can get within BIDS)
There was a problem hiding this comment.
@lzehl's suggested 'recording_id' and 'event_type'
I would recommend 'stream_name' unless some more rigorous structure elsewhere defines what the 'recording' means
There was a problem hiding this comment.
I would keep current trial_type to have the uniform "abuse" and rely on potential later rename
And then accompany it with recording_path or recording_desc (if not specific -- for description) alike to provide path (within nwb) or description
There was a problem hiding this comment.
Decided on 'stream_id' to be consistent with the channels table; this may come back but if it does we will change both at the same time
There was a problem hiding this comment.
Which will then be changed to 'stream_identifier' all in one go
Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>
yarikoptic
left a comment
There was a problem hiding this comment.
see my concern about (ab)using _events.tsv for offsets+durations of recordings
This addresses three of the unresolved documentation threads from @effigies on #2307. It targets
bep032-review, so it merges into #2307 rather than into master. No schema changes.Sidecar JSON Section
The prose said that setup metadata should be stored in a dedicated
Setupnode and recommended aProcedurenode holdingPharmaceuticals,Sample, andSupplementary. Neither node exists in the schema, and the sidecar rules inrules/sidecars/microephys.yamldefine all of these fields at the top level, so the prose described a nesting that the validator would not accept. I removed the references to nodes, stated that the sidecar is a flat JSON object and that the tables group keys by topic for readability only, and promoted the three former sub-subsections to the same level as the other sidecar tables. This follows Lyuba's reading of the thread (#2307 (comment)).Multiple Recordings in a Single Data File
This section ended with
<!-- TODO: Macro for events -->and promised a list of additionalevents.tsvcolumns. The schema does not define any microephys-specific events columns, and I do not think we need them: one row per recording, withonsetanddurationrelative to the start of the data file andtrial_typeas the label, is enough to locate each recording. I replaced the TODO with that description and a short example. If the group would rather define dedicated columns, that should be a separate schema PR, and I would suggest removing the section for now per Chris's comment.Examples of Real Datasets
The section still pointed to the NeuralEnsemble and sprenger GIN repositories, which are several years out of date with the current specification. It now lists the two bids-examples datasets (
microephys_toyfrom bids-standard/bids-examples#527 andmicroephys_ecephys_multielectrode_graspfrom bids-standard/bids-examples#491) and points to the bids-dandisets project for further real-world datasets, as Yarik and Sylvain suggested in the thread. The links into bids-examples assume those two PRs merge alongside this BEP. I also removed the GIN links from the intracellular toy dataset paragraph and replaced the stale "section 3.8.2" reference with an anchor link.🤖 Generated with Claude Code
https://claude.ai/code/session_01Wjq3vyBQs3SGtfrPRvp8Ws