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

Support reading of arbitrary fields #142

Merged
merged 12 commits into from
Jan 23, 2025
Merged

Support reading of arbitrary fields #142

merged 12 commits into from
Jan 23, 2025

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Jan 16, 2025

This PR is to prepare for #130

  • Changed the getGroupObjects method to getStorageObjects to allow for more general lookup of groups, datasets, and attributes. The current method is limited to look-up of groups within a group. The updated getStorageObjects now supports: 1) lookup starting from datasets and groups and 2) retrieval of groups, datasets, and attributes, 3) allows the user to specify what types of storage objects to retrieve.
  • Fixed an issue in the findTypes function, which previously only looked at Groups so typed objects that were datasets (e.g., VectorData) would have been ignored.
  • Added unit tests for findTypes and missing tests for RegisteredType and HDF5IO
  • Added new RegisteredType::readField function for reading arbitrary dataset and attribute fields
  • Added second variant of the RegisteredType::readField method to allow reading of arbitrary fields that are itself instances of RegisteredType, such as, reading VectorData from a DynamicTable or reading ElectricalSeries from an NWBFile

Fix #133
Fix #131

@oruebel oruebel requested a review from stephprince January 16, 2025 00:14
@oruebel oruebel marked this pull request as draft January 16, 2025 00:19
@oruebel oruebel removed the request for review from stephprince January 16, 2025 00:19
@oruebel oruebel marked this pull request as ready for review January 16, 2025 00:36
@oruebel oruebel requested a review from stephprince January 16, 2025 00:36
@oruebel
Copy link
Contributor Author

oruebel commented Jan 16, 2025

@stephprince sorry in case this caused confusion, but I needed the changes from #139 here for testing, and since #139 was not merged yet I combined it with this PR so I could run the tests. This PR is now ready for review.

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2025

Codecov Report

Attention: Patch coverage is 98.64865% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.96%. Comparing base (d76f8b7) to head (1a713e0).

Files with missing lines Patch % Lines
src/nwb/NWBFile.cpp 50.00% 2 Missing ⚠️
tests/testRegisteredType.cpp 97.43% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
+ Coverage   92.01%   92.96%   +0.94%     
==========================================
  Files          61       62       +1     
  Lines        4009     4278     +269     
  Branches      262      266       +4     
==========================================
+ Hits         3689     3977     +288     
+ Misses        310      291      -19     
  Partials       10       10              

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

@oruebel oruebel changed the title Update getGroupObjects to getStorageObjects Support reading of arbitrary fields Jan 19, 2025
stephprince
stephprince previously approved these changes Jan 23, 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.

Looks good to me! Added some minor suggestions.

docs/pages/userdocs/read.dox Outdated Show resolved Hide resolved
src/Types.hpp Outdated Show resolved Hide resolved
tests/testHDF5IO.cpp Show resolved Hide resolved
@oruebel
Copy link
Contributor Author

oruebel commented Jan 23, 2025

@stephprince can you please re-approve. I committed your suggestions and added the test case you had suggested in 19fecb4

@oruebel oruebel merged commit d71c561 into main Jan 23, 2025
10 checks passed
@oruebel oruebel deleted the general_obj_lookup branch January 23, 2025 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants