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 RegisteredTypes #143

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Conversation

oruebel
Copy link
Contributor

@oruebel oruebel commented Jan 19, 2025

Support reading typed columns of a DynamicTable and as a result support more broadly reading of RegisteredType objects via predefined functions via the new DEFINE_REGISTERED_FIELD macro, much like the existing DEFINE_FIELD macro. This PR makes the following key changes:

  • Changed Data and VectorData to take an optional template parameter with the data type of the dataset they represent. This is to allow us to predefine both the class and the data type to use for reading via the DEFINE_REGISTERED_FIELD macro as well as when we read VectorData via RegisteredType::create
  • Added new DEFINE_REGISTERED_FIELD macro. In contrast to the existing DEFINE_FIELD macro, this is not for reading datasets and attributes but for reading typed objects for which we have a RegisteredType that represents it
  • Updated the developer and user docs and the read example to show how reading of RegisteredType objects works with the new DEFINE_REGISTERED_FIELD macro available
  • Made the DEFINE_FIELD definitions in NWBFile public (they were private by error)
  • Added DEFINE_REGISTERED_FIELD(readElectrodeTable, ElectrodeTable, on NWBFile for reading the ElectrodeTable using the correct type
  • Fixed error in ElectrodeTable. The columns were written with a fixed-length string type of 250 characters instead of a variable-length string type
  • Added DEFINE_REGISTERED_FIELD definitions for reading the location and group_name columns of the ElectrodeTable
  • Updated DynamicTable::addColumn to avoid extra iteration since writeDataBlock can handle writing all the variable length strings in one call
  • Added DynamicTable::readColumn method to simplify reading of arbitrary columns
  • Added DEFINE_REGISTERED_FIELD(readIdColumn in DynamicTable to read the ElementIdentifiers
  • Updated and added unit tests

Fix #130
Fix #115

@oruebel
Copy link
Contributor Author

oruebel commented Jan 19, 2025

@stephprince I made this a separate PR from #142, because I: 1) didn't want to make that PR more complicated, 2) allow #142 to be merged independently, and 3) felt that the changes in this PR are a bit more fundamental and may require a closer look and maybe some discussion.

Base automatically changed from general_obj_lookup to main January 23, 2025 19:12
@codecov-commenter
Copy link

codecov-commenter commented Jan 23, 2025

Codecov Report

Attention: Patch coverage is 99.06542% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.27%. Comparing base (00a529c) to head (42e61b7).

Files with missing lines Patch % Lines
src/nwb/hdmf/table/DynamicTable.hpp 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #143      +/-   ##
==========================================
+ Coverage   93.20%   93.27%   +0.06%     
==========================================
  Files          62       60       -2     
  Lines        4328     4385      +57     
  Branches      268      269       +1     
==========================================
+ Hits         4034     4090      +56     
- Misses        284      285       +1     
  Partials       10       10              

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

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.

Read columns via DynamicTable Support read with classes using REGISTER_SUBCLASS_WITH_TYPENAME
2 participants