-
Notifications
You must be signed in to change notification settings - Fork 16
Add optional device link to TimeSeries #662
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
h-mayorquin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this moving forward. Two suggesions/questions on my side.
| data is calculated, the contents of 'sync' are mostly for archival purposes. | ||
| quantity: '?' | ||
| links: | ||
| - name: device |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - name: device | |
| - name: acquisition_device |
Maybe this would make the expected use more clear?
| links: | ||
| - name: device | ||
| target_type: Device | ||
| doc: Device used to record this time series. This should not be used to represent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| doc: Device used to record this time series. This should not be used to represent | |
| doc: > | |
| Teh device that directly acquired the samples stored in this TimeSeries. | |
| This field captures data acquisition provenance only and must refer to the | |
| device that digitized or otherwise recorded the signal. | |
| When multiple hardware components are involved (e.g., probe → headstage → | |
| acquisition system), this field must refer to the acquisition system itself, | |
| i.e., the device that digitized the data, not upstream sensors or | |
| hardware. | |
| This field must not be used to encode causal, control, or stimulation | |
| relationships. For example, it must not refer to devices that delivered | |
| optogenetic or electrical stimulation, triggered task events, or generated | |
| control signals. Such relationships should be represented elsewhere in the schema. |
Two sugggestions:
-
Clarify what happens in multi-device acquisition chains like in a typical ecephys setup where we have: probe/head-stage -> DAQ/acquisition box. I thinks this fit nicely there as we don't have a way of storing the DAQ (the probe/headstage can be encoded in the ElectrodeGroup device). The same or microscopy where the ImagingPlane captures the device but not the acquisition box that was used to acquire the data.
-
More concrete examples of what should not be here.
What do you think?
Summary of changes
TimeSeries#602Requires changes in PyNWB.
Checklist
For all schema changes:
docs/format/source/format_release_notes.rst.hdmf-common-schemapoints to the latest release and not the latest commit on themainbranch.If this is the first schema change after a schema release (i.e., the version string in
core/nwb.namespace.yamldoes notend in "-alpha"), then:
core/nwb.namespace.yamlandcore/nwb.file.yamlto the next major/minor/patchversion with the suffix "-alpha". For example, if the current version is 2.4.0 and this is a minor change, then the
new version string should be "2.5.0-alpha".
versionvariable indocs/format/source/conf.pyto the next version without thesuffix "-alpha", e.g., "2.5.0".
releasevariable indocs/format/source/conf.pyto the next version with the suffix"-alpha", e.g., "2.5.0-alpha".
docs/format/source/format_release_notes.rstfor the new versionwith the date "Upcoming" in parentheses.