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 to use the new TimeSeriesReferenceVectorData #484

Closed
rly opened this issue Aug 9, 2021 · 4 comments · Fixed by NeurodataWithoutBorders/pynwb#1390
Closed
Milestone

Comments

@rly
Copy link
Contributor

rly commented Aug 9, 2021

NWB 2.4.0 introduces the TimeSeriesReferenceVectorData neurodata type which is identical to the VectorData-typed column named "timeseries" in the TimeIntervals table. For consistency in the data format and API, the TimeIntervals neurodata type should be updated to use the TimeSeriesReferenceVectorData neurodata type for the column named "timeseries".

We will need to decide for the PyNWB API how to handle reading of old files written using the VectorData form of the "timeseries" column. Options:

  1. Leave the column as an object of type VectorData.
  2. Read the column data into an object of type TimeSeriesReferenceVectorData with the same object ID. This will likely require a change in the ObjectMapper base code to allow overriding the type-to-class mapping or modifying the builder before constructing the new object associated with the "timeseries" column.

ref: #483

@rly rly added this to the NWB 2.5.0 milestone Aug 9, 2021
@oruebel
Copy link
Contributor

oruebel commented Aug 9, 2021

Just adding another option:
3. Read as VectorData for NWB 2 - 2.3 files and read as TimeSeriesReferenceVectorData for NWB >=2.4 files. Optionally add a function TimeSeriesReferenceVectorData.from_vectordata to convert a VectorData column to a TimeSeriesReferenceVectorData if a user needs the new functionality. Similarly, we could have a TimeSeriesReference.from_tupes to convert the results returned from VectorData to a list of reference tuples. Optionally, there could then also be some corresponding function on TimeIntervals to update the type of the TimeIntervals['timeseries'] column if necessary. An advantage of this behavior is that it is explicit for the user and in most cases is probably not necessary anyways. The downside is that it's up to the user if they want to use new features for older files.

@oruebel
Copy link
Contributor

oruebel commented Aug 9, 2021

For the upcoming release my current thinking is that we should probably go with "Option 1" and leave TimeIntervals as is and then decide on an option to update the type after the release.

@rly
Copy link
Contributor Author

rly commented Aug 9, 2021

Yes, I agree with Option 1 for the upcoming 2.4.0 release. We can still implement options 2 and 3 later on.

@oruebel
Copy link
Contributor

oruebel commented Aug 11, 2021

NeurodataWithoutBorders/pynwb#1390 defines another alternative approach (option 4). Instead attempting to fix the issue in the ObjectMapper this approach updates the Container class for TimeIntervals.timeseries after the fact to change it inplace from a VectorData to a TimeSeriesReferenceVectorData

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