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

Add read tests for TimeSeries , VectorData, Data, ElementIdentifers, Device` #124

Merged
merged 25 commits into from
Jan 11, 2025

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Jan 9, 2025

Add unit tests for readfor TimeSeries For every neurodata_type we should test that: a) the type itself can be read from file, and b) that all DEFINE_FIELDS specified for read can be read.

  • Add base read test for TimeSeries

Fix #32 As part of writing the tests for TimeSeries, we should also double-check that all fields that AqNWB writes, that they are also exposed for read and that all types listed for read are also written.

  • Add missing TimeSeries.data.offset attribute on write
  • Add missing TimeSeries.data.continuity attribute on write
  • Add missing fields control and control_description datasets on write (read via readControlDescription andreadControl)
  • Add missing field starting_time dataset and corresponding attributes unit and rate on write (read via readStartingTime, readStartingTimeUnit, readStartingTimeRate
  • Move the creation of neurodata_type and namespace attributes to Container and add corresponding DEFINE_FIELDs for read

Fix #126 handling of Data and VectorData:

  • Moved creation of neurodata_type and namespace attributes to Data and added DEFINE_FIELD for those (similar to Container)
  • Added initalize() methods for Data and VectorData
  • Update DynamicTable and ElectrodeTable to call VectorData.initalize instead of setting up VectorData manually

Added write/read tests for additional types

  • VectorData with int, double, and string data
  • Data with a single type (the remaining is covered by the VectorData tests)
  • ElementIdentifiers and updated the class to overwrite the readData method to set the data type to int by default
  • Device

Additional fixes needed:

@oruebel oruebel mentioned this pull request Jan 9, 2025
73 tasks
@oruebel oruebel changed the base branch from main to add_read January 9, 2025 03:17
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.01%. Comparing base (44e957d) to head (b91dd24).

Additional details and impacted files
@@             Coverage Diff              @@
##           add_read     #124      +/-   ##
============================================
+ Coverage     90.87%   91.01%   +0.13%     
============================================
  Files            53       53              
  Lines          3289     3349      +60     
  Branches        229      229              
============================================
+ Hits           2989     3048      +59     
- Misses          290      291       +1     
  Partials         10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@oruebel oruebel changed the title Add read field read tests Add read tests for TimeSeries Jan 9, 2025
@oruebel oruebel marked this pull request as ready for review January 9, 2025 20:01
@oruebel
Copy link
Contributor Author

oruebel commented Jan 9, 2025

@stephprince this PR is now ready for you to take a look at

@oruebel oruebel requested a review from stephprince January 10, 2025 16:33
@oruebel oruebel changed the title Add read tests for TimeSeries Add read tests for TimeSeries and VectorData Jan 10, 2025
Copy link
Collaborator

@stephprince stephprince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added my review comments for the TimeSeries tests, but I have not gotten to the Vector Data or Table related changes yet

src/nwb/base/TimeSeries.cpp Outdated Show resolved Hide resolved
src/nwb/base/TimeSeries.hpp Outdated Show resolved Hide resolved
src/io/hdf5/HDF5IO.cpp Outdated Show resolved Hide resolved
tests/testTimeSeries.cpp Outdated Show resolved Hide resolved
tests/testTimeSeries.cpp Outdated Show resolved Hide resolved
@oruebel oruebel changed the title Add read tests for TimeSeries and VectorData Add read tests for TimeSeries , VectorData, Data, ElementIdentifers, Device` Jan 10, 2025
@stephprince
Copy link
Collaborator

Finished looking through the other files, other than the previous comments all looks good to me!

@oruebel oruebel merged commit 7730f29 into add_read Jan 11, 2025
9 checks passed
@oruebel oruebel deleted the add_read_field_read_tests branch January 11, 2025 00:20
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