Skip to content
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

Update TimeIntervals.timeseries to use new TimeSeriesReferenceVectorData #486

Merged
merged 6 commits into from
Apr 26, 2022

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Aug 12, 2021

Motivation

#470 added the new TimeSeriesReferenceVectorData type. This PR updates TimeIntervals to use the new TimeSeriesReferenceVectorData type. This does not alter the overall structure of TimeIntervals in a major way aside from changing the value of the neurodata_type attribute in the file from VectorData to TimeSeriesReferenceVectorData. This change replaces the existing TimeIntervals.timeseries column with a TimeSeriesReferenceVectorData type column of the same name and overall schema. This change facilitate creating common functionality around TimeSeriesReferenceVectorData. This change affects all existing TimeIntervals tables as part of the intervals/ group, i.e., intervals/epochs, intervals/trials, and intervals/invalid_times (#470)

Summary of changes

  • Update TimeIntervals to use the new TimeSeriesReferenceVectorData type

Relates PRs

NeurodataWithoutBorders/pynwb#1390 is the matching PR on PyNWB for this

PR checklist for schema changes

  • Update the version string in docs/format/source/conf.py, core/nwb.namespace.yaml, and /core/nwb.file.yaml
    to the next version with the suffix "-alpha"
  • Add a new section in the release notes for the new version with the date "Upcoming"
  • Add release notes for the PR to docs/format/source/format_release_notes.rst

@oruebel oruebel requested a review from rly August 12, 2021 23:10
@rly rly added this to the NWB 2.5.0 milestone Aug 12, 2021
@rly
Copy link
Contributor

rly commented Aug 12, 2021

Looks good. Please update the version strings in docs/format/source/conf.py, core/nwb.namespace.yaml, and /core/nwb.file.yaml to "2.5.0-alpha"

@oruebel oruebel marked this pull request as ready for review August 12, 2021 23:42
rly
rly previously approved these changes Aug 13, 2021
@rly
Copy link
Contributor

rly commented Aug 13, 2021

Looks good. Feel free to squash and merge.

rly
rly previously approved these changes Apr 20, 2022
@rly rly mentioned this pull request Apr 20, 2022
5 tasks
@CodyCBakerPhD
Copy link
Contributor

Would this sort of reference object make sense to use for waveform columns on a UnitsTable? We often have difficulty getting the properties of that column to properly match TimeSeries-like attributes. What do you think @oruebel @bendichter

This change facilitate creating common functionality around TimeSeriesReferenceVectorData. This change affects all existing TimeIntervals tables as part of the intervals/ group, i.e., intervals/epochs, intervals/trials, and intervals/invalid_times

Would it also apply to custom TimeInterval table objects placed in, say, a 'processing' module?

@oruebel
Copy link
Contributor Author

oruebel commented Apr 20, 2022

Would it also apply to custom TimeInterval table objects placed in, say, a 'processing' module?

Yes, this updates the TimeIntervals type and as such will affect all TimeIntervals tables

Would this sort of reference object make sense to use for waveform columns on a UnitsTable?

I'm not sure. This would require storing the waveforms in (likely many individual) TimeSeries and then referencing subsets of those TimeSeries. I'm not sure how well this would work here, in particular, I'm not sure about how many different TimeSeries objects would be required and what the performance for accessing the data would end up being as it will required resolving a lot of links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants