From e015a1d55a9d868399d61e65fa1d36d4ecde151b Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Tue, 14 Jan 2025 15:11:16 -0800 Subject: [PATCH 01/19] Add unit tests for BaseIO::findTypes --- tests/CMakeLists.txt | 1 + tests/testBaseIO.cpp | 107 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) create mode 100644 tests/testBaseIO.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index c11a2242..45e90f33 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -12,6 +12,7 @@ include(Catch) # ---- Tests ---- add_executable(aqnwb_test + testBaseIO.cpp testData.cpp testDevice.cpp testEcephys.cpp diff --git a/tests/testBaseIO.cpp b/tests/testBaseIO.cpp new file mode 100644 index 00000000..dbd29b77 --- /dev/null +++ b/tests/testBaseIO.cpp @@ -0,0 +1,107 @@ +#include + +#include "io/hdf5/HDF5IO.hpp" +#include "testUtils.hpp" + +using namespace AQNWB::IO; + +TEST_CASE("Test findTypes functionality", "[BaseIO]") +{ + std::string filename = getTestFilePath("test_findTypes.h5"); + HDF5::HDF5IO io(filename); + io.open(FileMode::Overwrite); + + SECTION("Empty file returns empty result") + { + auto result = + io.findTypes("/", {"core::NWBFile"}, SearchMode::STOP_ON_TYPE); + REQUIRE(result.empty()); + } + + SECTION("Single type at root") + { + // Create root group with type attributes + io.createGroup("/"); + io.createAttribute("core", "/", "namespace"); + io.createAttribute("NWBFile", "/", "neurodata_type"); + + auto result = + io.findTypes("/", {"core::NWBFile"}, SearchMode::STOP_ON_TYPE); + REQUIRE(result.size() == 1); + REQUIRE(result["/"] == "core::NWBFile"); + } + + SECTION("Multiple nested types with STOP_ON_TYPE") + { + // Setup hierarchy + io.createGroup("/"); + io.createAttribute("core", "/", "namespace"); + io.createAttribute("NWBFile", "/", "neurodata_type"); + + io.createGroup("/testProcessingModule"); + io.createAttribute("core", "/testProcessingModule", "namespace"); + io.createAttribute( + "ProcessingModule", "/testProcessingModule", "neurodata_type"); + + auto result = io.findTypes("/", + {"core::NWBFile", "core::ProcessingModule"}, + SearchMode::STOP_ON_TYPE); + REQUIRE(result.size() == 1); + REQUIRE(result["/"] == "core::NWBFile"); + } + + SECTION("Multiple nested types with CONTINUE_ON_TYPE") + { + // Setup hierarchy + io.createGroup("/"); + io.createAttribute("core", "/", "namespace"); + io.createAttribute("NWBFile", "/", "neurodata_type"); + + io.createGroup("/testProcessingModule"); + io.createAttribute("core", "/testProcessingModule", "namespace"); + io.createAttribute( + "ProcessingModule", "/testProcessingModule", "neurodata_type"); + + auto result = io.findTypes("/", + {"core::NWBFile", "core::ProcessingModule"}, + SearchMode::CONTINUE_ON_TYPE); + REQUIRE(result.size() == 2); + REQUIRE(result["/"] == "core::NWBFile"); + REQUIRE(result["/testProcessingModule"] == "core::ProcessingModule"); + } + + SECTION("Non-matching types are not included") + { + // Setup hierarchy + io.createGroup("/"); + io.createAttribute("core", "/", "namespace"); + io.createAttribute("NWBFile", "/", "neurodata_type"); + + io.createGroup("/testProcessingModule"); + io.createAttribute("core", "/testProcessingModule", "namespace"); + io.createAttribute( + "ProcessingModule", "/testProcessingModule", "neurodata_type"); + + auto result = + io.findTypes("/", {"core::Device"}, SearchMode::CONTINUE_ON_TYPE); + REQUIRE(result.empty()); + } + + SECTION("Missing attributes are handled gracefully") + { + // Create group with missing neurodata_type attribute + io.createGroup("/"); + io.createAttribute("core", "/", "namespace"); + + io.createGroup("/testProcessingModule"); + io.createAttribute("core", "/testProcessingModule", "namespace"); + io.createAttribute( + "ProcessingModule", "/testProcessingModule", "neurodata_type"); + + auto result = io.findTypes( + "/", {"core::ProcessingModule"}, SearchMode::CONTINUE_ON_TYPE); + REQUIRE(result.size() == 1); + REQUIRE(result["/testProcessingModule"] == "core::ProcessingModule"); + } + io.close(); +} \ No newline at end of file From f3321650f7ebf8a60179ae0d8b7985641dda43d0 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Tue, 14 Jan 2025 15:27:53 -0800 Subject: [PATCH 02/19] Add unit tests for unsupported data type --- tests/testHDF5IO.cpp | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/tests/testHDF5IO.cpp b/tests/testHDF5IO.cpp index c3ccac88..8d24f0e9 100644 --- a/tests/testHDF5IO.cpp +++ b/tests/testHDF5IO.cpp @@ -1437,8 +1437,29 @@ TEST_CASE("HDF5IO; read dataset", "[hdf5io]") SECTION("read unsupported data type") { - // TODO Add a test that tries to read some unsupported data type - // such as a strange compound data type + // open file + std::string path = getTestFilePath("test_ReadUnsupportedDataType.h5"); + std::shared_ptr hdf5io = + std::make_shared(path); + hdf5io->open(); + + // Create a compound datatype + H5::CompType compoundType(sizeof(double) * 2); + compoundType.insertMember("real", 0, H5::PredType::NATIVE_DOUBLE); + compoundType.insertMember( + "imag", sizeof(double), H5::PredType::NATIVE_DOUBLE); + + // Create dataset with compound type directly using HDF5 C++ API + H5::H5File file(path, H5F_ACC_RDWR); + hsize_t dims[1] = {5}; + H5::DataSpace dataspace(1, dims); + H5::DataSet dataset = + file.createDataSet("ComplexData", compoundType, dataspace); + + // Attempt to read the dataset - should throw an exception + REQUIRE_THROWS_AS(hdf5io->readDataset("/ComplexData"), std::runtime_error); + + hdf5io->close(); } } @@ -1547,4 +1568,4 @@ TEST_CASE("HDF5IO; read dataset subset", "[hdf5io]") hdf5io->close(); } -} \ No newline at end of file +} From 1568e97a4d5b4a4887b6e28cbccfaf6cead2f00f Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Tue, 14 Jan 2025 16:30:32 -0800 Subject: [PATCH 03/19] Enhance testing for RegisteredType --- tests/testRegisteredType.cpp | 126 +++++++++++++++++++++++++++++++---- 1 file changed, 114 insertions(+), 12 deletions(-) diff --git a/tests/testRegisteredType.cpp b/tests/testRegisteredType.cpp index f1f8c28b..4dcd0b97 100644 --- a/tests/testRegisteredType.cpp +++ b/tests/testRegisteredType.cpp @@ -10,6 +10,40 @@ using namespace AQNWB::NWB; +// Test class with custom type name +class CustomNameType : public RegisteredType +{ +public: + CustomNameType(const std::string& path, std::shared_ptr io) + : RegisteredType(path, io) + { + } + + virtual std::string getTypeName() const override { return "CustomType"; } + virtual std::string getNamespace() const override { return "test"; } +}; + +// Test class with field definitions +class TestFieldType : public RegisteredType +{ +public: + TestFieldType(const std::string& path, std::shared_ptr io) + : RegisteredType(path, io) + { + } + + virtual std::string getTypeName() const override { return "TestFieldType"; } + virtual std::string getNamespace() const override { return "test"; } + + DEFINE_FIELD(testAttribute, + AttributeField, + int32_t, + "test_attr", + "Test attribute field") + DEFINE_FIELD( + testDataset, DatasetField, float, "test_dataset", "Test dataset field") +}; + TEST_CASE("RegisterType", "[base]") { SECTION("test that the registry is working") @@ -27,23 +61,14 @@ TEST_CASE("RegisterType", "[base]") auto registry = RegisteredType::getRegistry(); auto factoryMap = RegisteredType::getFactoryMap(); // TODO we are checking for at least 10 registered types because that is how - // many - // were defined at the time of implementation of this test. We know we - // will add more, but we would like to avoid having to update this test - // every time, so we are only checking for at least 10 + // many were defined at the time of implementation of this test. We know we + // will add more, but we would like to avoid having to update this test + // every time, so we are only checking for at least 10 REQUIRE(registry.size() >= 10); REQUIRE(factoryMap.size() >= 10); REQUIRE(registry.size() == factoryMap.size()); // Test that we can indeed instantiate all registered types - // This also ensures that each factory function works correctly, - // and hence, that all subtypes implement the expected constructor - // for the RegisteredType::create method. This is similar to - // checking: - // for (const auto& pair : factoryMap) { - // auto instance = pair.second(examplePath, io); - // REQUIRE(instance != nullptr); - // } std::cout << "Registered Types:" << std::endl; for (const auto& entry : factoryMap) { const std::string& subclassFullName = entry.first; @@ -73,6 +98,9 @@ TEST_CASE("RegisterType", "[base]") // Check that the examplePath is set as expected REQUIRE(instance->getPath() == examplePath); + + // Test getFullName + REQUIRE(instance->getFullName() == (typeNamespace + "::" + typeName)); } } @@ -137,4 +165,78 @@ TEST_CASE("RegisterType", "[base]") std::string readTSType = readContainer->getTypeName(); REQUIRE(readTSType == "TimeSeries"); } + + SECTION("test error handling for invalid type creation") + { + std::string filename = getTestFilePath("testInvalidType.h5"); + std::shared_ptr io = std::make_unique(filename); + std::string examplePath("/example/path"); + + // Test creating with non-existent type name + auto invalidInstance = + RegisteredType::create("invalid::Type", examplePath, io); + REQUIRE(invalidInstance == nullptr); + + // Test creating with empty type name + auto emptyInstance = RegisteredType::create("", examplePath, io); + REQUIRE(emptyInstance == nullptr); + + // Test creating with malformed type name (missing namespace) + auto malformedInstance = + RegisteredType::create("NoNamespace", examplePath, io); + REQUIRE(malformedInstance == nullptr); + } + + SECTION("test custom type name") + { + std::string filename = getTestFilePath("testCustomType.h5"); + std::shared_ptr io = std::make_unique(filename); + std::string examplePath("/example/path"); + + // Create instance of custom named type + auto customInstance = std::make_shared(examplePath, io); + REQUIRE(customInstance != nullptr); + REQUIRE(customInstance->getTypeName() == "CustomType"); + REQUIRE(customInstance->getNamespace() == "test"); + REQUIRE(customInstance->getFullName() == "test::CustomType"); + } + + SECTION("test field definitions") + { + std::string filename = getTestFilePath("testFields.h5"); + std::shared_ptr io = std::make_unique(filename); + io->open(); + std::string examplePath("/test_fields"); + + // Create test instance + auto testInstance = std::make_shared(examplePath, io); + REQUIRE(testInstance != nullptr); + + // Create parent group + io->createGroup(examplePath); + + // Create test data + const int32_t attrValue = 42; + const std::vector datasetValues = {1.0f, 2.0f, 3.0f}; + + // Write test data + io->createAttribute( + BaseDataType::I32, &attrValue, examplePath, "test_attr"); + io->createArrayDataSet(BaseDataType::F32, + SizeArray {3}, + SizeArray {3}, + examplePath + "/test_dataset"); + + // Test attribute field + auto attrWrapper = testInstance->testAttribute(); + REQUIRE(attrWrapper != nullptr); + auto attrData = attrWrapper->values(); + REQUIRE(attrData.data[0] == attrValue); + + // Test dataset field + auto datasetWrapper = testInstance->testDataset(); + REQUIRE(datasetWrapper != nullptr); + + io->close(); + } } From 6ee040c9ba7147b9de1e10a98952e38b3f0213cd Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Wed, 15 Jan 2025 16:11:30 -0800 Subject: [PATCH 04/19] Update getGroupObjects to getStorageObjects --- src/io/BaseIO.cpp | 21 ++++++--- src/io/BaseIO.hpp | 23 +++++---- src/io/hdf5/HDF5IO.cpp | 53 +++++++++++++++++++-- src/io/hdf5/HDF5IO.hpp | 23 +++++---- src/nwb/NWBFile.cpp | 7 +-- tests/testHDF5IO.cpp | 105 +++++++++++++++++++++++++++++++++++------ 6 files changed, 188 insertions(+), 44 deletions(-) diff --git a/src/io/BaseIO.cpp b/src/io/BaseIO.cpp index d2d7f05f..18cbe8f7 100644 --- a/src/io/BaseIO.cpp +++ b/src/io/BaseIO.cpp @@ -77,7 +77,6 @@ std::unordered_map BaseIO::findTypes( { // Check if the current object exists as a dataset or group if (objectExists(current_path)) { - std::cout << "Current Path: " << current_path << std::endl; // Check if we have a typed object if (attributeExists(current_path + "/neurodata_type") && attributeExists(current_path + "/namespace")) @@ -92,8 +91,6 @@ std::unordered_map BaseIO::findTypes( std::string full_type = namespace_attr.data[0] + "::" + neurodata_type_attr.data[0]; - std::cout << "Full name: " << full_type << std::endl; - // Check if the full type matches any of the given types if (types.find(full_type) != types.end()) { found_types[current_path] = full_type; @@ -103,9 +100,14 @@ std::unordered_map BaseIO::findTypes( // object if (search_mode == SearchMode::CONTINUE_ON_TYPE) { // Get the list of objects inside the current group - std::vector objects = getGroupObjects(current_path); + std::vector> objects = + getStorageObjects(current_path, StorageObjectType::Undefined); for (const auto& obj : objects) { - searchTypes(AQNWB::mergePaths(current_path, obj)); + if (obj.second == StorageObjectType::Group + || obj.second == StorageObjectType::Dataset) + { + searchTypes(AQNWB::mergePaths(current_path, obj.first)); + } } } } catch (...) { @@ -117,9 +119,14 @@ std::unordered_map BaseIO::findTypes( else { // Get the list of objects inside the current group - std::vector objects = getGroupObjects(current_path); + std::vector> objects = + getStorageObjects(current_path, StorageObjectType::Undefined); for (const auto& obj : objects) { - searchTypes(AQNWB::mergePaths(current_path, obj)); + if (obj.second == StorageObjectType::Group + || obj.second == StorageObjectType::Dataset) + { + searchTypes(AQNWB::mergePaths(current_path, obj.first)); + } } } } diff --git a/src/io/BaseIO.hpp b/src/io/BaseIO.hpp index c6df0a89..c5b7a1fd 100644 --- a/src/io/BaseIO.hpp +++ b/src/io/BaseIO.hpp @@ -223,19 +223,26 @@ class BaseIO virtual bool attributeExists(const std::string& path) const = 0; /** - * @brief Gets the list of objects inside a group. + * @brief Gets the list of storage objects (groups, datasets, attributes) + * inside a group. * - * This function returns a vector of relative paths of all objects inside - * the specified group. If the input path is not a group (e.g., as dataset - * or attribute or invalid object), then an empty list should be - * returned. + * This function returns the relative paths and storage type of all objects + * inside the specified group. If the input path is an attribute then an empty + * list should be returned. If the input path is a dataset, then only the + * attributes will be returned. Note, this function is not recursive, i.e., + * it only looks for storage objects associated directly with the given path. * * @param path The path to the group. + * @param objectType Define which types of storage object to look for, i.e., + * only objects of this specified type will be returned. * - * @return A vector of relative paths of all objects inside the group. + * @return A vector of pairs of relative paths and their corresponding storage + * object types. */ - virtual std::vector getGroupObjects( - const std::string& path) const = 0; + virtual std::vector> + getStorageObjects(const std::string& path, + const StorageObjectType& objectType = + StorageObjectType::Undefined) const = 0; /** * @brief Finds all datasets and groups of the given types in the HDF5 file. diff --git a/src/io/hdf5/HDF5IO.cpp b/src/io/hdf5/HDF5IO.cpp index 2bbd9482..2313b05f 100644 --- a/src/io/hdf5/HDF5IO.cpp +++ b/src/io/hdf5/HDF5IO.cpp @@ -989,16 +989,61 @@ bool HDF5IO::attributeExists(const std::string& path) const return (attributePtr != nullptr); } -std::vector HDF5IO::getGroupObjects(const std::string& path) const +std::vector> +HDF5IO::getStorageObjects(const std::string& path, + const StorageObjectType& objectType) const + { - std::vector objects; - if (getH5ObjectType(path) == H5O_TYPE_GROUP) { + std::vector> objects; + + H5O_type_t h5Type = getH5ObjectType(path); + if (h5Type == H5O_TYPE_GROUP) { H5::Group group = m_file->openGroup(path); hsize_t num_objs = group.getNumObjs(); for (hsize_t i = 0; i < num_objs; ++i) { - objects.push_back(group.getObjnameByIdx(i)); + std::string objName = group.getObjnameByIdx(i); + H5G_obj_t objType = group.getObjTypeByIdx(i); + StorageObjectType storageObjectType; + switch (objType) { + case H5G_GROUP: + storageObjectType = StorageObjectType::Group; + break; + case H5G_DATASET: + storageObjectType = StorageObjectType::Dataset; + break; + default: + storageObjectType = StorageObjectType::Undefined; + } + if (storageObjectType == objectType + || objectType == StorageObjectType::Undefined) + { + objects.emplace_back(objName, storageObjectType); + } + } + + // Include attributes for groups + if (objectType == StorageObjectType::Attribute + || objectType == StorageObjectType::Undefined) + { + SizeType numAttrs = group.getNumAttrs(); + for (SizeType i = 0; i < numAttrs; ++i) { + H5::Attribute attr = group.openAttribute(i); + objects.emplace_back(attr.getName(), StorageObjectType::Attribute); + } + } + } else if (h5Type == H5O_TYPE_DATASET) { + if (objectType == StorageObjectType::Attribute + || objectType == StorageObjectType::Undefined) + { + H5::DataSet dataset = m_file->openDataSet(path); + SizeType numAttrs = dataset.getNumAttrs(); + for (SizeType i = 0; i < numAttrs; ++i) { + H5::Attribute attr = dataset.openAttribute(i); + objects.emplace_back(attr.getName(), StorageObjectType::Attribute); + } } } + return objects; } diff --git a/src/io/hdf5/HDF5IO.hpp b/src/io/hdf5/HDF5IO.hpp index 45c7663a..f0f00cfd 100644 --- a/src/io/hdf5/HDF5IO.hpp +++ b/src/io/hdf5/HDF5IO.hpp @@ -294,19 +294,26 @@ class HDF5IO : public BaseIO bool attributeExists(const std::string& path) const override; /** - * @brief Gets the list of objects inside a group. + * @brief Gets the list of storage objects (groups, datasets, attributes) + * inside a group. * - * This function returns a vector of relative paths of all objects inside - * the specified group. If the input path is not a group (e.g., as dataset - * or attribute or invalid object), then an empty list should be - * returned. + * This function returns the relative paths and storage type of all objects + * inside the specified group. If the input path is an attribute then an empty + * list should be returned. If the input path is a dataset, then only the + * attributes will be returned. Note, this function is not recursive, i.e., + * it only looks for storage objects associated directly with the given path. * * @param path The path to the group. + * @param objectType Define which types of storage object to look for, i.e., + * only objects of this specified type will be returned. * - * @return A vector of relative paths of all objects inside the group. + * @return A vector of pairs of relative paths and their corresponding storage + * object types. */ - std::vector getGroupObjects( - const std::string& path) const override; + virtual std::vector> + getStorageObjects(const std::string& path, + const StorageObjectType& objectType = + StorageObjectType::Undefined) const override; /** * @brief Returns the HDF5 type of object at a given path. diff --git a/src/nwb/NWBFile.cpp b/src/nwb/NWBFile.cpp index 4d3b4743..6fc1f649 100644 --- a/src/nwb/NWBFile.cpp +++ b/src/nwb/NWBFile.cpp @@ -65,7 +65,8 @@ Status NWBFile::initialize(const std::string& identifierText, bool NWBFile::isInitialized() const { - auto existingGroupObjects = m_io->getGroupObjects("/"); + std::vector> existingGroupObjects = + m_io->getStorageObjects("/", StorageObjectType::Group); if (existingGroupObjects.size() == 0) { return false; } @@ -84,8 +85,8 @@ bool NWBFile::isInitialized() const // Iterate over the existing objects and add to foundObjects if it's a // required object for (const auto& obj : existingGroupObjects) { - if (requiredObjects.find(obj) != requiredObjects.end()) { - foundObjects.insert(obj); + if (requiredObjects.find(obj.first) != requiredObjects.end()) { + foundObjects.insert(obj.first); } } diff --git a/tests/testHDF5IO.cpp b/tests/testHDF5IO.cpp index f01479e3..6f9264f7 100644 --- a/tests/testHDF5IO.cpp +++ b/tests/testHDF5IO.cpp @@ -128,21 +128,21 @@ TEST_CASE("createGroup", "[hdf5io]") } } -TEST_CASE("getGroupObjects", "[hdf5io]") +TEST_CASE("getStorageObjects", "[hdf5io]") { // create and open file - std::string filename = getTestFilePath("test_getGroupObjects.h5"); - IO::HDF5::HDF5IO hdf5io(filename); + std::string filename = getTestFilePath("test_getStorageObjects.h5"); + AQNWB::IO::HDF5::HDF5IO hdf5io(filename); hdf5io.open(); SECTION("empty group") { hdf5io.createGroup("/emptyGroup"); - auto groupContent = hdf5io.getGroupObjects("/emptyGroup"); + auto groupContent = hdf5io.getStorageObjects("/emptyGroup"); REQUIRE(groupContent.size() == 0); } - SECTION("group with datasets and subgroups") + SECTION("group with datasets, subgroups, and attributes") { hdf5io.createGroup("/data"); hdf5io.createGroup("/data/subgroup1"); @@ -152,15 +152,43 @@ TEST_CASE("getGroupObjects", "[hdf5io]") hdf5io.createArrayDataSet( BaseDataType::I32, SizeArray {0}, SizeArray {1}, "/data/dataset2"); - auto groupContent = hdf5io.getGroupObjects("/data"); - REQUIRE(groupContent.size() == 4); - REQUIRE(std::find(groupContent.begin(), groupContent.end(), "subgroup1") + // Add attributes to the group + int attrData1 = 42; + hdf5io.createAttribute(BaseDataType::I32, &attrData1, "/data", "attr1"); + int attrData2 = 43; + hdf5io.createAttribute(BaseDataType::I32, &attrData2, "/data", "attr2"); + + auto groupContent = hdf5io.getStorageObjects("/data"); + REQUIRE(groupContent.size() == 6); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("subgroup1"), + StorageObjectType::Group)) + != groupContent.end()); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("subgroup2"), + StorageObjectType::Group)) != groupContent.end()); - REQUIRE(std::find(groupContent.begin(), groupContent.end(), "subgroup2") + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("dataset1"), + StorageObjectType::Dataset)) != groupContent.end()); - REQUIRE(std::find(groupContent.begin(), groupContent.end(), "dataset1") + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("dataset2"), + StorageObjectType::Dataset)) != groupContent.end()); - REQUIRE(std::find(groupContent.begin(), groupContent.end(), "dataset2") + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("attr1"), + StorageObjectType::Attribute)) + != groupContent.end()); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("attr2"), + StorageObjectType::Attribute)) != groupContent.end()); } @@ -169,17 +197,66 @@ TEST_CASE("getGroupObjects", "[hdf5io]") hdf5io.createGroup("/rootGroup1"); hdf5io.createGroup("/rootGroup2"); - auto groupContent = hdf5io.getGroupObjects("/"); + auto groupContent = hdf5io.getStorageObjects("/"); REQUIRE(groupContent.size() == 2); - REQUIRE(std::find(groupContent.begin(), groupContent.end(), "rootGroup1") + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("rootGroup1"), + StorageObjectType::Group)) + != groupContent.end()); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("rootGroup2"), + StorageObjectType::Group)) != groupContent.end()); - REQUIRE(std::find(groupContent.begin(), groupContent.end(), "rootGroup2") + } + + SECTION("filter by object type") + { + hdf5io.createGroup("/filterGroup"); + hdf5io.createGroup("/filterGroup/subgroup1"); + hdf5io.createArrayDataSet(BaseDataType::I32, + SizeArray {0}, + SizeArray {1}, + "/filterGroup/dataset1"); + + // Add attributes to the group + int attrData = 44; + hdf5io.createAttribute( + BaseDataType::I32, &attrData, "/filterGroup", "attr1"); + + auto groupContent = + hdf5io.getStorageObjects("/filterGroup", StorageObjectType::Group); + REQUIRE(groupContent.size() == 1); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("subgroup1"), + StorageObjectType::Group)) + != groupContent.end()); + + groupContent = + hdf5io.getStorageObjects("/filterGroup", StorageObjectType::Dataset); + REQUIRE(groupContent.size() == 1); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("dataset1"), + StorageObjectType::Dataset)) + != groupContent.end()); + + groupContent = + hdf5io.getStorageObjects("/filterGroup", StorageObjectType::Attribute); + REQUIRE(groupContent.size() == 1); + REQUIRE(std::find(groupContent.begin(), + groupContent.end(), + std::make_pair(std::string("attr1"), + StorageObjectType::Attribute)) != groupContent.end()); } // close file hdf5io.close(); } +// END TEST_CASE("getStorageObjects", "[hdf5io]") TEST_CASE("HDF5IO; write datasets", "[hdf5io]") { From 9fc3e86099cfa87da6f1db7441c64d7ff0c1f15c Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Wed, 15 Jan 2025 16:26:34 -0800 Subject: [PATCH 05/19] Add unit test for findType with Dataset --- tests/testBaseIO.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/testBaseIO.cpp b/tests/testBaseIO.cpp index dbd29b77..01f16b67 100644 --- a/tests/testBaseIO.cpp +++ b/tests/testBaseIO.cpp @@ -31,6 +31,21 @@ TEST_CASE("Test findTypes functionality", "[BaseIO]") REQUIRE(result["/"] == "core::NWBFile"); } + SECTION("Search for dataset type") + { + // Create root group with type attributes + io.createGroup("/"); + io.createArrayDataSet( + BaseDataType::I32, SizeArray {0}, SizeArray {1}, "/dataset1"); + io.createAttribute("hdmf-common", "/dataset1", "namespace"); + io.createAttribute("VectorData", "/dataset1", "neurodata_type"); + + auto result = io.findTypes( + "/", {"hdmf-common::VectorData"}, SearchMode::STOP_ON_TYPE); + REQUIRE(result.size() == 1); + REQUIRE(result["/dataset1"] == "hdmf-common::VectorData"); + } + SECTION("Multiple nested types with STOP_ON_TYPE") { // Setup hierarchy From b9fcb4c001dc807c1a863bd4ebeb934ddd5fae2d Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 16:18:07 -0800 Subject: [PATCH 06/19] Fix #131 support reading of custom fields via RegisteredType --- docs/pages/userdocs/read.dox | 27 +++++++++++++- src/Types.hpp | 13 +++++++ src/io/BaseIO.hpp | 4 +- src/nwb/RegisteredType.hpp | 45 +++++++++++++++++++++++ tests/examples/test_ecephys_data_read.cpp | 24 ++++++++++++ tests/testRegisteredType.cpp | 37 +++++++++++++++++-- 6 files changed, 142 insertions(+), 8 deletions(-) diff --git a/docs/pages/userdocs/read.dox b/docs/pages/userdocs/read.dox index 363ac471..38156850 100644 --- a/docs/pages/userdocs/read.dox +++ b/docs/pages/userdocs/read.dox @@ -308,7 +308,7 @@ * - \ref AQNWB::IO::BaseIO "BaseIO", \ref AQNWB::IO::HDF5::HDF5IO "HDF5IO" are responsible for * i) reading type attribute and group information, ii) searching the file for typed objects via * \ref AQNWB::IO::BaseIO::findTypes "findTypes()" methods, and iii) retrieving the paths of all - * object within a group via \ref AQNWB::IO::BaseIO::getGroupObjects "getGroupObjects()" + * object associated with a storage objects (e.g., a Group) via \ref AQNWB::IO::BaseIO::getStorageObjects "getStoragebjects()" * * \subsubsection read_design_wrapper_registeredType RegisteredType * @@ -320,7 +320,9 @@ * methods that we can use to instantiate any registered subclass just using the ``io`` object * and ``path`` for the object in the file. \ref AQNWB::NWB::RegisteredType "RegisteredType" can read * the type information from the corresponding `namespace` and `neurodata_type` attributes to - * determine the full type and in run look up the corresponding class in its registry and create the type. + * determine the full type, then look up the corresponding class in its registry, and then create the type. + * Using \ref AQNWB::NWB::RegisteredType::readField "RegisteredType::readField" also provides a + * general mechanism for reading arbitrary fields. * * \subsubsection read_design_wrapper_subtypes Child classes of RegisteredType (e.g., Container) * @@ -540,5 +542,26 @@ * * \snippet tests/examples/test_ecephys_data_read.cpp example_read_only_stringattr_snippet * + * + * \subsubsection read_design_example_read_arbitrary_field Reading arbitrary fields + * + * Even if there is no dedicated `DEFINE_FIELD` definition available, we can still read + * any arbitrary sub-field associated with a particular \ref AQNWB::NWB::RegisteredType "RegisteredType" + * via the generic \ref AQNWB::NWB::RegisteredType::readField "RegisteredType::readField" method. The main + * difference is that for datasets and attributes we need to specify all the additional information + * (e.g., the relative path, object type, and data type) ourselves, whereas using `DEFINE_FIELD` + * this information has already been specified for us. For example, to read the data from + * the \ref AQNWB::NWB::ElectricalSeries "ElectricalSeries" we can call: + * + * \snippet tests/examples/test_ecephys_data_read.cpp example_read_generic_dataset_field_snippet + * + * Similarly, we can also read any sub-fields that are itself \ref AQNWB::NWB::RegisteredType "RegisteredType" + * objects via \ref AQNWB::NWB::RegisteredType::readField "RegisteredType::readField" (e.g., to read custom + * \ref AQNWB::NWB::VectorData "VectorData" columns of a \ref AQNWB::NWB::DynamicTable "DynamicTable"). In + * contrast to dataset and attribute fields, we here only need to specify the relative path of the field. + * \ref AQNWB::NWB::RegisteredType "RegisteredType" in turn can read the type information from the + * `neurodata_type` and `namespace` attributes in the file directly. + * + * \snippet tests/examples/test_ecephys_data_read.cpp example_read_generic_registeredtype_field_snippet */ diff --git a/src/Types.hpp b/src/Types.hpp index 03e7789f..17ebbdfc 100644 --- a/src/Types.hpp +++ b/src/Types.hpp @@ -35,6 +35,19 @@ class Types Undefined = -1 }; + /** + * \brief Helper struct to check if a value is a data field, i.e., + * Dataset or Attribute + * + * This function is used to enforce constrains on templated functions that + * should only be callable for valid StorageObjectType values + */ + template + struct IsDataStorageObjectType + : std::integral_constant + { + }; + /** * @brief Alias for the size type used in the project. */ diff --git a/src/io/BaseIO.hpp b/src/io/BaseIO.hpp index c5b7a1fd..e44d780c 100644 --- a/src/io/BaseIO.hpp +++ b/src/io/BaseIO.hpp @@ -96,12 +96,12 @@ enum class SearchMode /** * @brief Stop searching inside an object once a matching type is found. */ - STOP_ON_TYPE, + STOP_ON_TYPE = 1, /** * @brief Continue searching inside an object even after a matching type is * found. */ - CONTINUE_ON_TYPE + CONTINUE_ON_TYPE = 2, }; /** diff --git a/src/nwb/RegisteredType.hpp b/src/nwb/RegisteredType.hpp index 4f71b61c..d39e203a 100644 --- a/src/nwb/RegisteredType.hpp +++ b/src/nwb/RegisteredType.hpp @@ -198,6 +198,51 @@ class RegisteredType return (getNamespace() + "::" + getTypeName()); } + /** + * @brief Support reading of arbitrary fields by their relative path + * + * This function provided as a general "backup" to support reading of + * arbitrary fields even if the sub-class may not have an explicit + * DEFINE_FIELD specified for the field. If a DEFINE_FIELD exists then + * the corresponding read method should be used as it avoids the need + * for specifying most (if not all) of the function an template + * parameteres needed by this function. + * + * @param fieldPath The relative path of the field within the current type, + * i.e., relative to `m_path` + * @tparam SOT The storage object type. This must be a either + * StorageObjectType::Dataset or StorageObjectType::Attribute + * @tparam VTYPE The value type of the field to be read. + * @tparam Enable SFINAE (Substitution Failure Is Not An Error) mechanism + * to enable this function only if SOT is a Dataset or Attribute. + * + * @return ReadDataWrapper object for lazy reading of the field + */ + template::value, + int>::type = 0> + inline std::unique_ptr> readField( + const std::string& fieldPath) const + { + return std::make_unique>( + m_io, AQNWB::mergePaths(m_path, fieldPath)); + } + + /** + * @brief Read a field that is itself a RegisteredType + * + * @param fieldPath The relative path of the field within the current type, + * i.e., relative to `m_path. The field must itself be RegisteredType + * + * @return A unique_ptr to the created instance of the subclass. + */ + inline std::shared_ptr readField( + const std::string& fieldPath) + { + return this->create(AQNWB::mergePaths(m_path, fieldPath), m_io); + } + protected: /** * @brief Register a subclass name and its factory function in the registry. diff --git a/tests/examples/test_ecephys_data_read.cpp b/tests/examples/test_ecephys_data_read.cpp index 916cc17c..ab791973 100644 --- a/tests/examples/test_ecephys_data_read.cpp +++ b/tests/examples/test_ecephys_data_read.cpp @@ -268,5 +268,29 @@ TEST_CASE("ElectricalSeriesReadExample", "[ecephys]") readElectricalSeries->readDataUnit()->values().data[0]; REQUIRE(esUnitValue == std::string("volts")); // [example_read_only_stringattr_snippet] + + // [example_read_generic_dataset_field_snippet] + // Read the data field via the generic readField method + auto readElectricalSeriesData3 = + readElectricalSeries->readField( + std::string("data")); + // Read the data values as usual + DataBlock readDataValues3 = readElectricalSeriesData3->values(); + REQUIRE(readDataValues3.data.size() == (numSamples * numChannels)); + // [example_read_generic_dataset_field_snippet] + + // [example_read_generic_registeredtype_field_snippet] + // read the NWBFile + auto readNWBFile = + NWB::RegisteredType::create("/", readio); + // read the ElectricalSeries from the NWBFile object via the readField + // method returning a generic std::shared_ptr + auto readRegisteredType = readNWBFile->readField(esdata_path); + // cast the generic pointer to the more specific ElectricalSeries + std::shared_ptr readElectricalSeries2 = + std::dynamic_pointer_cast( + readRegisteredType); + REQUIRE(readElectricalSeries2 != nullptr); + // [example_read_generic_registeredtype_field_snippet] } } diff --git a/tests/testRegisteredType.cpp b/tests/testRegisteredType.cpp index b280b8ac..a5f9efcd 100644 --- a/tests/testRegisteredType.cpp +++ b/tests/testRegisteredType.cpp @@ -163,6 +163,15 @@ TEST_CASE("RegisterType", "[base]") auto readTS = AQNWB::NWB::RegisteredType::create(examplePath, io); std::string readTSType = readContainer->getTypeName(); REQUIRE(readTSType == "TimeSeries"); + + // Attempt to read the TimeSeries using the generic readField method + // By providing an empty path we tell readField to read itself + std::shared_ptr readRegisteredType = + readContainer->readField(std::string("")); + REQUIRE(readRegisteredType != nullptr); + std::shared_ptr readRegisteredTypeAsTimeSeries = + std::dynamic_pointer_cast(readRegisteredType); + REQUIRE(readRegisteredTypeAsTimeSeries != nullptr); } SECTION("test error handling for invalid type creation") @@ -221,10 +230,13 @@ TEST_CASE("RegisterType", "[base]") // Write test data io->createAttribute( BaseDataType::I32, &attrValue, examplePath, "test_attr"); - io->createArrayDataSet(BaseDataType::F32, - SizeArray {3}, - SizeArray {3}, - examplePath + "/test_dataset"); + auto datasetRecordingData = + io->createArrayDataSet(BaseDataType::F32, + SizeArray {3}, + SizeArray {3}, + examplePath + "/test_dataset"); + datasetRecordingData->writeDataBlock( + SizeArray {3}, SizeArray {0}, BaseDataType::F32, datasetValues.data()); // Test attribute field auto attrWrapper = testInstance->testAttribute(); @@ -235,6 +247,23 @@ TEST_CASE("RegisterType", "[base]") // Test dataset field auto datasetWrapper = testInstance->testDataset(); REQUIRE(datasetWrapper != nullptr); + auto datasetData = datasetWrapper->values(); + REQUIRE(datasetData.data == datasetValues); + + // Test reading using the general readField method + // Read test_attr via readField + auto attrWrapper2 = testInstance->readField( + std::string("test_attr")); + REQUIRE(attrWrapper2 != nullptr); + auto attrData2 = attrWrapper2->values(); + REQUIRE(attrData2.data[0] == attrValue); + + // Read test_dataset via readField + auto datasetWrapper2 = testInstance->readField( + std::string("test_dataset")); + REQUIRE(datasetWrapper2 != nullptr); + auto datasetData2 = datasetWrapper2->values(); + REQUIRE(datasetData2.data == datasetValues); io->close(); } From e33469ba23964760e3034c47404434d55553f81f Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 16:33:30 -0800 Subject: [PATCH 07/19] Fix codespell --- src/nwb/RegisteredType.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nwb/RegisteredType.hpp b/src/nwb/RegisteredType.hpp index d39e203a..2d7b1fd0 100644 --- a/src/nwb/RegisteredType.hpp +++ b/src/nwb/RegisteredType.hpp @@ -206,7 +206,7 @@ class RegisteredType * DEFINE_FIELD specified for the field. If a DEFINE_FIELD exists then * the corresponding read method should be used as it avoids the need * for specifying most (if not all) of the function an template - * parameteres needed by this function. + * parameters needed by this function. * * @param fieldPath The relative path of the field within the current type, * i.e., relative to `m_path` From 589da5da41120d9ad6db50d1109004a68d246561 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 22:18:16 -0800 Subject: [PATCH 08/19] Support reading VectorData columns with data type defined in the schema --- docs/Doxyfile.in | 1 + src/nwb/RegisteredType.hpp | 40 +++++++++++++++++- src/nwb/file/ElectrodeTable.cpp | 14 ++++--- src/nwb/file/ElectrodeTable.hpp | 16 +++++++- src/nwb/hdmf/base/Data.cpp | 36 ++++++++-------- src/nwb/hdmf/base/Data.hpp | 23 ++++++++--- src/nwb/hdmf/table/DynamicTable.cpp | 34 +++++++++++---- src/nwb/hdmf/table/DynamicTable.hpp | 35 +++++++++++++--- src/nwb/hdmf/table/ElementIdentifiers.hpp | 10 +++-- src/nwb/hdmf/table/VectorData.cpp | 36 ++++++++-------- src/nwb/hdmf/table/VectorData.hpp | 32 ++++++++++++--- tests/testData.cpp | 6 +-- tests/testFile.cpp | 50 ++++++++++++++++++++++- tests/testVectorData.cpp | 14 ++++--- 14 files changed, 262 insertions(+), 85 deletions(-) diff --git a/docs/Doxyfile.in b/docs/Doxyfile.in index 1216659d..774a3c21 100644 --- a/docs/Doxyfile.in +++ b/docs/Doxyfile.in @@ -16,6 +16,7 @@ PROJECT_NUMBER = "@PROJECT_VERSION@" # simplified expansion of the macro for documentation purposes MACRO_EXPANSION = YES PREDEFINED += "DEFINE_FIELD(name, storageObjectType, default_type, fieldPath, description)=/** description */ template inline std::unique_ptr> name() const;" +PREDEFINED += "DEFINE_REGISTERED_FIELD(name, registeredType, fieldPath, description)=/** description */ inline std::shared_ptr name() const;" EXPAND_ONLY_PREDEF = YES diff --git a/src/nwb/RegisteredType.hpp b/src/nwb/RegisteredType.hpp index 2d7b1fd0..ef16cbf2 100644 --- a/src/nwb/RegisteredType.hpp +++ b/src/nwb/RegisteredType.hpp @@ -238,7 +238,7 @@ class RegisteredType * @return A unique_ptr to the created instance of the subclass. */ inline std::shared_ptr readField( - const std::string& fieldPath) + const std::string& fieldPath) const { return this->create(AQNWB::mergePaths(m_path, fieldPath), m_io); } @@ -336,7 +336,7 @@ class RegisteredType * @brief Defines a lazy-loaded field accessor function. * * This macro generates a function that returns a lazy-loaded wrapper for a - * field. + * dataset or attribute field. * * \note * The Doxyfile.in defines a simplified expansion of this function @@ -370,5 +370,41 @@ class RegisteredType m_io, AQNWB::mergePaths(m_path, fieldPath)); \ } +/** + * @brief Defines a lazy-loaded accessor function for reading fields that are + * RegisteredTypes + * + * This macro generates a function that returns the approbriate subtype of + * RegisteredType, e.g., to read VectorData from a DynamicTable or a + * TimeSeries from and NWBFile. + * + * \note + * The Doxyfile.in defines a simplified expansion of this function + * for generating the documentation for the autogenerated function. + * This means: 1) When updating the macro here, we also need to ensure + * that the expansion in the Doxyfile.in is still accurate and 2) the + * docstring that is defined by the macro here is not being used by + * Doxygen but the version generated by its on PREDEFINED expansion. + * + * @param name The name of the function to generate. + * @param registeredType The specific subclass of registered type to use + * @param fieldPath The path to the field. + * @param description A detailed description of the field. + */ +#define DEFINE_REGISTERED_FIELD(name, registeredType, fieldPath, description) \ + /** \ + * @brief Returns the instance of the class representing the ##name field. \ + * \ + * @return A shared pointer to an instance of ##registeredType representing \ + * the object \ + * \ + * description \ + */ \ + inline std::shared_ptr name() const \ + { \ + return RegisteredType::create( \ + AQNWB::mergePaths(m_path, fieldPath), m_io); \ + } + } // namespace NWB } // namespace AQNWB diff --git a/src/nwb/file/ElectrodeTable.cpp b/src/nwb/file/ElectrodeTable.cpp index 5494bdd7..b7c81f10 100644 --- a/src/nwb/file/ElectrodeTable.cpp +++ b/src/nwb/file/ElectrodeTable.cpp @@ -16,9 +16,9 @@ ElectrodeTable::ElectrodeTable(std::shared_ptr io) {"group", "group_name", "location"}) , m_electrodeDataset(std::make_unique( AQNWB::mergePaths(electrodeTablePath, "id"), io)) - , m_groupNamesDataset(std::make_unique( + , m_groupNamesDataset(std::make_unique>( AQNWB::mergePaths(electrodeTablePath, "group_name"), io)) - , m_locationsDataset(std::make_unique( + , m_locationsDataset(std::make_unique>( AQNWB::mergePaths(electrodeTablePath, "location"), io)) { } @@ -28,9 +28,9 @@ ElectrodeTable::ElectrodeTable(const std::string& path, : DynamicTable(electrodeTablePath, io) , m_electrodeDataset(std::make_unique( AQNWB::mergePaths(electrodeTablePath, "id"), io)) - , m_groupNamesDataset(std::make_unique( + , m_groupNamesDataset(std::make_unique>( AQNWB::mergePaths(electrodeTablePath, "group_name"), io)) - , m_locationsDataset(std::make_unique( + , m_locationsDataset(std::make_unique>( AQNWB::mergePaths(electrodeTablePath, "location"), io)) { std::cerr << "ElectrodeTable object is required to appear at " @@ -46,6 +46,8 @@ void ElectrodeTable::initialize(const std::string& description) { // create group DynamicTable::initialize(description); + IO::BaseDataType vstrType(IO::BaseDataType::Type::V_STR, + 0); // 0 indicates variable length m_electrodeDataset->initialize(std::unique_ptr( m_io->createArrayDataSet(IO::BaseDataType::I32, @@ -54,14 +56,14 @@ void ElectrodeTable::initialize(const std::string& description) AQNWB::mergePaths(m_path, "id")))); m_groupNamesDataset->initialize( std::unique_ptr( - m_io->createArrayDataSet(IO::BaseDataType::STR(250), + m_io->createArrayDataSet(vstrType, SizeArray {0}, SizeArray {1}, AQNWB::mergePaths(m_path, "group_name"))), "the name of the ElectrodeGroup this electrode is a part of"); m_locationsDataset->initialize( std::unique_ptr( - m_io->createArrayDataSet(IO::BaseDataType::STR(250), + m_io->createArrayDataSet(vstrType, SizeArray {0}, SizeArray {1}, AQNWB::mergePaths(m_path, "location"))), diff --git a/src/nwb/file/ElectrodeTable.hpp b/src/nwb/file/ElectrodeTable.hpp index 1676a73a..455211ce 100644 --- a/src/nwb/file/ElectrodeTable.hpp +++ b/src/nwb/file/ElectrodeTable.hpp @@ -81,6 +81,18 @@ class ElectrodeTable : public DynamicTable inline const static std::string electrodeTablePath = "/general/extracellular_ephys/electrodes"; + DEFINE_REGISTERED_FIELD( + readLocationColumn, + VectorData, + "location", + "the location of channel within the subject e.g. brain region") + + DEFINE_REGISTERED_FIELD( + readGroupNameColumn, + VectorData, + "group_name", + "the name of the ElectrodeGroup this electrode is a part of") + private: /** * @brief The global indices for each electrode. @@ -116,11 +128,11 @@ class ElectrodeTable : public DynamicTable /** * @brief The group names column for write */ - std::unique_ptr m_groupNamesDataset; + std::unique_ptr> m_groupNamesDataset; /** * @brief The locations column for write */ - std::unique_ptr m_locationsDataset; + std::unique_ptr> m_locationsDataset; }; } // namespace AQNWB::NWB diff --git a/src/nwb/hdmf/base/Data.cpp b/src/nwb/hdmf/base/Data.cpp index 6708ac6f..27f1cbe4 100644 --- a/src/nwb/hdmf/base/Data.cpp +++ b/src/nwb/hdmf/base/Data.cpp @@ -1,21 +1,23 @@ #include "nwb/hdmf/base/Data.hpp" -using namespace AQNWB::NWB; - -// Container -// Initialize the static registered_ member to trigger registration -REGISTER_SUBCLASS_IMPL(Data) - -/** Constructor */ -Data::Data(const std::string& path, std::shared_ptr io) - : RegisteredType(path, io) +namespace AQNWB::NWB { -} +// Register the base Data type (with default std::any) for use with +// RegisteredType::create +template<> +REGISTER_SUBCLASS_IMPL(Data) -void Data::initialize(std::unique_ptr&& dataset) -{ - m_dataset = std::move(dataset); - // setup common attributes - m_io->createCommonNWBAttributes( - m_path, this->getNamespace(), this->getTypeName()); -} \ No newline at end of file +// Explicitly instantiate for all common data types +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +template class Data; +} // namespace AQNWB::NWB diff --git a/src/nwb/hdmf/base/Data.hpp b/src/nwb/hdmf/base/Data.hpp index 936bc78f..727da973 100644 --- a/src/nwb/hdmf/base/Data.hpp +++ b/src/nwb/hdmf/base/Data.hpp @@ -9,12 +9,14 @@ namespace AQNWB::NWB { /** * @brief An abstract data type for a dataset. + * @tparam DTYPE The data type of the data managed by Data */ +template class Data : public RegisteredType { public: // Register Data class as a registered type - REGISTER_SUBCLASS(Data, "hdmf-common") + REGISTER_SUBCLASS_WITH_TYPENAME(Data, "hdmf-common", "Data") /** * @brief Constructor. @@ -22,12 +24,15 @@ class Data : public RegisteredType * @param path The path of the container. * @param io A shared pointer to the IO object. */ - Data(const std::string& path, std::shared_ptr io); + Data(const std::string& path, std::shared_ptr io) + : RegisteredType(path, io) + { + } /** - * @brief Destructor. + * @brief Virtual destructor. */ - ~Data() {} + virtual ~Data() override {} /** * @brief Initialize the dataset for the Data object @@ -37,7 +42,13 @@ class Data : public RegisteredType * * @param dataset The rvalue unique pointer to the BaseRecordingData object */ - void initialize(std::unique_ptr&& dataset); + void initialize(std::unique_ptr&& dataset) + { + m_dataset = std::move(dataset); + // setup common attributes + m_io->createCommonNWBAttributes( + m_path, this->getNamespace(), this->getTypeName()); + } /** * @brief Check whether the m_dataset has been initialized @@ -47,7 +58,7 @@ class Data : public RegisteredType std::unique_ptr m_dataset; // Define the data fields to expose for lazy read access - DEFINE_FIELD(readData, DatasetField, std::any, "", The main data) + DEFINE_FIELD(readData, DatasetField, DTYPE, "", The main data) DEFINE_FIELD(readNeurodataType, AttributeField, diff --git a/src/nwb/hdmf/table/DynamicTable.cpp b/src/nwb/hdmf/table/DynamicTable.cpp index 08f03f6e..5bb8c1da 100644 --- a/src/nwb/hdmf/table/DynamicTable.cpp +++ b/src/nwb/hdmf/table/DynamicTable.cpp @@ -30,19 +30,22 @@ void DynamicTable::initialize(const std::string& description) } /** Add column to table */ -void DynamicTable::addColumn(std::unique_ptr& vectorData, - const std::vector& values) +void DynamicTable::addColumn( + std::unique_ptr>& vectorData, + const std::vector& values) { if (!vectorData->isInitialized()) { std::cerr << "VectorData dataset is not initialized" << std::endl; } else { // write in loop because variable length string - for (SizeType i = 0; i < values.size(); i++) - vectorData->m_dataset->writeDataBlock( - std::vector {1}, - std::vector {i}, - IO::BaseDataType::STR(values[i].size() + 1), - values); // TODO - add tests for this + IO::BaseDataType vstrType(IO::BaseDataType::Type::V_STR, + 0); // 0 indicates variable length + // for (SizeType i = 0; i < values.size(); i++) + vectorData->m_dataset->writeDataBlock( + std::vector {values.size()}, + std::vector {0}, + vstrType, // IO::BaseDataType::STR(values[i].size() + 1), + values); // TODO - add tests for this } } @@ -71,9 +74,22 @@ void DynamicTable::addReferenceColumn(const std::string& name, } else { std::string columnPath = AQNWB::mergePaths(m_path, name); m_io->createReferenceDataSet(columnPath, values); - auto refColumn = AQNWB::NWB::VectorData(columnPath, m_io); + auto refColumn = AQNWB::NWB::VectorData(columnPath, m_io); refColumn.initialize(nullptr, // Use nullptr because we only want to create // the attributes but not modify the data colDescription); } } + +/*template +std::shared_ptr> DynamicTable::readColumn( + const std::string& colName) +{ + std::string columnPath = AQNWB::mergePaths(m_path, colName); + if (m_io->objectExists(columnPath)) { + if (m_io->getStorageObjectType(columnPath) == StorageObjectType::Dataset) { + return std::make_shared>(columnPath, m_io); + } + } + return nullptr; +}*/ \ No newline at end of file diff --git a/src/nwb/hdmf/table/DynamicTable.hpp b/src/nwb/hdmf/table/DynamicTable.hpp index e1f42871..25cc13d4 100644 --- a/src/nwb/hdmf/table/DynamicTable.hpp +++ b/src/nwb/hdmf/table/DynamicTable.hpp @@ -54,7 +54,7 @@ class DynamicTable : public Container * @param vectorData A unique pointer to the `VectorData` dataset. * @param values The vector of string values. */ - void addColumn(std::unique_ptr& vectorData, + void addColumn(std::unique_ptr>& vectorData, const std::vector& values); /** @@ -84,6 +84,29 @@ class DynamicTable : public Container m_colNames = newColNames; } + /** + * @brief Read an arbitrary column of the DyanmicTable + * + * For columns defined in the schema the correspoding DEFINE_REGISTERED_FIELD + * read functions are preferred because they help avoid the need for + * specifying the specific name of the column and data type to use. + * + * @return The VectorData object representing the column or a nullptr if the + * column doesn't exists + */ + template + std::shared_ptr> readColumn(const std::string& colName) + { + std::string columnPath = AQNWB::mergePaths(m_path, colName); + if (m_io->objectExists(columnPath)) { + if (m_io->getStorageObjectType(columnPath) == StorageObjectType::Dataset) + { + return std::make_shared>(columnPath, m_io); + } + } + return nullptr; + } + DEFINE_FIELD(readColNames, AttributeField, std::string, @@ -96,11 +119,11 @@ class DynamicTable : public Container "description", Description of what is in this dynamic table) - DEFINE_FIELD(readId, - DatasetField, - std::any, - "id", - Array of unique identifiers for the rows of this dynamic table) + DEFINE_REGISTERED_FIELD( + readIdColumn, + ElementIdentifiers, + "id", + "unique identifiers for the rows of this dynamic table") protected: /** diff --git a/src/nwb/hdmf/table/ElementIdentifiers.hpp b/src/nwb/hdmf/table/ElementIdentifiers.hpp index 41212b24..a9acf169 100644 --- a/src/nwb/hdmf/table/ElementIdentifiers.hpp +++ b/src/nwb/hdmf/table/ElementIdentifiers.hpp @@ -8,10 +8,10 @@ namespace AQNWB::NWB * @brief A list of unique identifiers for values within a dataset, e.g. rows of * a DynamicTable. */ -class ElementIdentifiers : public Data +class ElementIdentifiers : public Data { public: - // Register Data class as a registered type + // Register ElementIdentifiers class as a registered type REGISTER_SUBCLASS(ElementIdentifiers, "hdmf-common") /** @@ -22,7 +22,9 @@ class ElementIdentifiers : public Data */ ElementIdentifiers(const std::string& path, std::shared_ptr io); - // Define the data fields to expose for lazy read access - DEFINE_FIELD(readData, DatasetField, int, "", The data identifiers) + /** + * @brief Virtual destructor. + */ + virtual ~ElementIdentifiers() override {} }; } // namespace AQNWB::NWB diff --git a/src/nwb/hdmf/table/VectorData.cpp b/src/nwb/hdmf/table/VectorData.cpp index 673ccfaf..2740ba82 100644 --- a/src/nwb/hdmf/table/VectorData.cpp +++ b/src/nwb/hdmf/table/VectorData.cpp @@ -1,21 +1,23 @@ #include "nwb/hdmf/table/VectorData.hpp" -using namespace AQNWB::NWB; - -// Initialize the static registered_ member to trigger registration -REGISTER_SUBCLASS_IMPL(VectorData) - -/** Constructor */ -VectorData::VectorData(const std::string& path, - std::shared_ptr io) - : Data(path, io) +namespace AQNWB::NWB { -} +// Register the base VectorData type (with default std::any) for use with +// RegisteredType::create +template<> +REGISTER_SUBCLASS_IMPL(VectorData) -void VectorData::initialize( - std::unique_ptr&& dataset, - const std::string& description) -{ - Data::initialize(std::move(dataset)); - m_io->createAttribute(description, m_path, "description"); -} +// Explicitly instantiate for all common data types +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +template class VectorData; +} // namespace AQNWB::NWB diff --git a/src/nwb/hdmf/table/VectorData.hpp b/src/nwb/hdmf/table/VectorData.hpp index 6fce6342..dfc4faa0 100644 --- a/src/nwb/hdmf/table/VectorData.hpp +++ b/src/nwb/hdmf/table/VectorData.hpp @@ -10,20 +10,32 @@ namespace AQNWB::NWB { /** * @brief An n-dimensional dataset representing a column of a DynamicTable. + * @tparam DTYPE The data type of the data managed by VectorData */ -class VectorData : public Data +template +class VectorData : public Data { public: - // Register Data class as a registered type - REGISTER_SUBCLASS(VectorData, "hdmf-common") - /** * @brief Constructor. * * @param path The path of the container. * @param io A shared pointer to the IO object. */ - VectorData(const std::string& path, std::shared_ptr io); + VectorData(const std::string& path, std::shared_ptr io) + : Data(path, io) + { + } + + // Register VectorData class as a registered type + REGISTER_SUBCLASS_WITH_TYPENAME(VectorData, + "hdmf-common", + "VectorData") + + /** + * @brief Virtual destructor. + */ + virtual ~VectorData() override {} /** * @brief Initialize the dataset for the Data object @@ -35,7 +47,15 @@ class VectorData : public Data * @param description The description of the VectorData */ void initialize(std::unique_ptr&& dataset, - const std::string& description); + const std::string& description) + { + Data::initialize(std::move(dataset)); + this->m_io->createAttribute(description, this->m_path, "description"); + } + + // Define the data fields to expose for lazy read access + using RegisteredType::m_io; + using RegisteredType::m_path; DEFINE_FIELD(readDescription, AttributeField, diff --git a/tests/testData.cpp b/tests/testData.cpp index 0d4ee18c..d7e68b77 100644 --- a/tests/testData.cpp +++ b/tests/testData.cpp @@ -34,7 +34,7 @@ TEST_CASE("Data", "[base]") io->createArrayDataSet(dataType, dataShape, chunking, dataPath); // setup Data object - NWB::Data columnData = NWB::Data(dataPath, io); + NWB::Data columnData = NWB::Data(dataPath, io); columnData.initialize(std::move(columnDataset)); // Write data to file @@ -49,8 +49,8 @@ TEST_CASE("Data", "[base]") readio->open(FileMode::ReadOnly); // Read all fields using the standard read methods - auto readRegisteredType = NWB::RegisteredType::create(dataPath, readio); - auto readData = std::dynamic_pointer_cast(readRegisteredType); + auto readData = + NWB::RegisteredType::create>(dataPath, readio); // Read the "namespace" attribute via the readNamespace field auto namespaceData = readData->readNamespace(); diff --git a/tests/testFile.cpp b/tests/testFile.cpp index 7e26e9d9..c9f33418 100644 --- a/tests/testFile.cpp +++ b/tests/testFile.cpp @@ -6,13 +6,14 @@ #include "io/hdf5/HDF5IO.hpp" #include "io/hdf5/HDF5RecordingData.hpp" #include "nwb/file/ElectrodeTable.hpp" +#include "nwb/hdmf/table/ElementIdentifiers.hpp" #include "testUtils.hpp" using namespace AQNWB; TEST_CASE("ElectrodeTable", "[ecephys]") { - SECTION("test initialization") + SECTION("test initialization and read") { std::string filename = getTestFilePath("electrodeTable.h5"); std::shared_ptr io = std::make_unique(filename); @@ -44,6 +45,53 @@ TEST_CASE("ElectrodeTable", "[ecephys]") std::vector read_channels(buffer, buffer + numChannels); delete[] buffer; REQUIRE(channelIDs == read_channels); + + // Test reading the location data + auto readLocation = electrodeTable.readLocationColumn(); + REQUIRE(readLocation != nullptr); + auto readLocationData = readLocation->readData(); + auto readLocationValues = readLocationData->values().data; + REQUIRE(readLocationValues.size() == 3); + std::vector expectedLocations = { + "unknown", "unknown", "unknown"}; + REQUIRE(readLocationValues == expectedLocations); + + // Test reading the groupName data + auto readGroupName = electrodeTable.readGroupNameColumn(); + REQUIRE(readGroupName != nullptr); + auto readGroupNameData = readGroupName->readData(); + auto readGroupNameValues = readGroupNameData->values().data; + REQUIRE(readGroupNameValues.size() == 3); + std::vector expectedGroupNames = { + "array0", "array0", "array0"}; + REQUIRE(readGroupNameValues == expectedGroupNames); + + // Test reading the id column + std::shared_ptr readId = + electrodeTable.readIdColumn(); + REQUIRE(readId != nullptr); + auto readIdData = readId->readData(); + auto readIdValues = readIdData->values().data; + REQUIRE(readIdValues.size() == 3); + std::vector expectedIdValues = {0, 1, 2}; + REQUIRE(readIdValues == expectedIdValues); + + // Test reading columns via the generic readColumn method + auto readGroupName2 = electrodeTable.readColumn("group_name"); + REQUIRE(readGroupName2 != nullptr); + auto readGroupNameData2 = readGroupName2->readData(); + auto readGroupNameValues2 = readGroupNameData2->values().data; + REQUIRE(readGroupNameValues.size() == 3); + REQUIRE(readGroupNameValues == expectedGroupNames); + + // Test reading id column via the generic readColumn method as VectorData + std::shared_ptr> readId2 = + electrodeTable.readColumn("id"); + REQUIRE(readId2 != nullptr); + auto readIdData2 = readId2->readData(); + auto readIdValues2 = readIdData2->values().data; + REQUIRE(readIdValues.size() == 3); + REQUIRE(readIdValues == expectedIdValues); } SECTION("test initialization with empty channels") diff --git a/tests/testVectorData.cpp b/tests/testVectorData.cpp index a8ca3061..17972352 100644 --- a/tests/testVectorData.cpp +++ b/tests/testVectorData.cpp @@ -35,7 +35,7 @@ TEST_CASE("VectorData", "[base]") io->createArrayDataSet(dataType, dataShape, chunking, dataPath); // setup VectorData object - NWB::VectorData columnVectorData = NWB::VectorData(dataPath, io); + NWB::VectorData columnVectorData = NWB::VectorData(dataPath, io); columnVectorData.initialize(std::move(columnDataset), description); // Write data to file @@ -52,7 +52,7 @@ TEST_CASE("VectorData", "[base]") // Read all fields using the standard read methods auto readRegisteredType = NWB::RegisteredType::create(dataPath, readio); auto readVectorData = - std::dynamic_pointer_cast(readRegisteredType); + std::make_shared>(dataPath, readio); // Read the "namespace" attribute via the readNamespace field auto namespaceData = readVectorData->readNamespace(); @@ -100,7 +100,8 @@ TEST_CASE("VectorData", "[base]") io->createArrayDataSet(dataType, dataShape, chunking, dataPath); // setup VectorData object - NWB::VectorData columnVectorData = NWB::VectorData(dataPath, io); + NWB::VectorData columnVectorData = + NWB::VectorData(dataPath, io); columnVectorData.initialize(std::move(columnDataset), description); // Write data to file @@ -117,7 +118,7 @@ TEST_CASE("VectorData", "[base]") // Read all fields using the standard read methods auto readRegisteredType = NWB::RegisteredType::create(dataPath, readio); auto readVectorData = - std::dynamic_pointer_cast(readRegisteredType); + std::make_shared>(dataPath, readio); // Read the "namespace" attribute via the readNamespace field auto namespaceData = readVectorData->readNamespace(); @@ -173,7 +174,8 @@ TEST_CASE("VectorData", "[base]") io->createArrayDataSet(dataType, dataShape, chunking, dataPath); // setup VectorData object - NWB::VectorData columnVectorData = NWB::VectorData(dataPath, io); + NWB::VectorData columnVectorData = + NWB::VectorData(dataPath, io); columnVectorData.initialize(std::move(columnDataset), description); // Write data to file @@ -190,7 +192,7 @@ TEST_CASE("VectorData", "[base]") // Read all fields using the standard read methods auto readRegisteredType = NWB::RegisteredType::create(dataPath, readio); auto readVectorData = - std::dynamic_pointer_cast(readRegisteredType); + std::make_shared>(dataPath, readio); // Read the "namespace" attribute via the readNamespace field auto namespaceData = readVectorData->readNamespace(); From 7e33d553ef7b0e9179634c3f9b27d4625e600192 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 22:27:12 -0800 Subject: [PATCH 09/19] Allow custom template type for DEFINE_REGISTERED_FIELD macro --- docs/Doxyfile.in | 2 +- src/nwb/RegisteredType.hpp | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/docs/Doxyfile.in b/docs/Doxyfile.in index 774a3c21..b8dd029f 100644 --- a/docs/Doxyfile.in +++ b/docs/Doxyfile.in @@ -16,7 +16,7 @@ PROJECT_NUMBER = "@PROJECT_VERSION@" # simplified expansion of the macro for documentation purposes MACRO_EXPANSION = YES PREDEFINED += "DEFINE_FIELD(name, storageObjectType, default_type, fieldPath, description)=/** description */ template inline std::unique_ptr> name() const;" -PREDEFINED += "DEFINE_REGISTERED_FIELD(name, registeredType, fieldPath, description)=/** description */ inline std::shared_ptr name() const;" +PREDEFINED += "DEFINE_REGISTERED_FIELD(name, registeredType, fieldPath, description)=/** description */ template inline std::shared_ptr name() const;" EXPAND_ONLY_PREDEF = YES diff --git a/src/nwb/RegisteredType.hpp b/src/nwb/RegisteredType.hpp index ef16cbf2..8d04beed 100644 --- a/src/nwb/RegisteredType.hpp +++ b/src/nwb/RegisteredType.hpp @@ -395,15 +395,20 @@ class RegisteredType /** \ * @brief Returns the instance of the class representing the ##name field. \ * \ + * @tparam RTYPE The RegisteredType of the field (default: ##registeredType) \ + * In most cases this should not be changed. But in the case of templated \ + * types, e.g,. VectorData a user may want to change this to a \ + * more specific subtype to use, e.g., VectorData \ * @return A shared pointer to an instance of ##registeredType representing \ * the object \ * \ * description \ */ \ - inline std::shared_ptr name() const \ + template \ + inline std::shared_ptr name() const \ { \ - return RegisteredType::create( \ - AQNWB::mergePaths(m_path, fieldPath), m_io); \ + return RegisteredType::create(AQNWB::mergePaths(m_path, fieldPath), \ + m_io); \ } } // namespace NWB From 4a1c4609ab8ad3fdeadebfe924a17228c6afea2a Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 23:19:43 -0800 Subject: [PATCH 10/19] Update read examples and docs --- docs/pages/devdocs/registered_types.dox | 41 ++++++++++-- docs/pages/userdocs/read.dox | 19 +++++- src/nwb/NWBFile.hpp | 79 ++++++++++++----------- src/nwb/hdmf/table/DynamicTable.hpp | 2 +- tests/examples/test_ecephys_data_read.cpp | 19 +++++- 5 files changed, 110 insertions(+), 50 deletions(-) diff --git a/docs/pages/devdocs/registered_types.dox b/docs/pages/devdocs/registered_types.dox index 6d4d8ae1..1f01bc6d 100644 --- a/docs/pages/devdocs/registered_types.dox +++ b/docs/pages/devdocs/registered_types.dox @@ -56,6 +56,15 @@ * DEFINE_FIELD(getData, DatasetField, float, "data", The main data) * @endcode * + * 6. Similarly, we use the \ref DEFINE_REGISTERED_FIELD macro to define getter methods for other + * \ref AQNWB::NWB::RegisteredType "RegisteredType" objects that we own, such as a + * \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable" that owns + * predefined \ref AQNWB::NWB::VectorData "VectorData" columns: + * @code + * DEFINE_REGISTERED_FIELD(readGroupNameColumn, VectorData, "group_name", "the name of the ElectrodeGroup") + * @endcode + * + * * \warning * To ensure proper function on read, the name of the class should match the name of the * ``neurodata_type`` as defined in the schema. Similarly, "my-namespace" should match @@ -80,11 +89,12 @@ * the indicated typename instead of the classname. * * \note - * ``DEFINE_FIELD`` creates templated, non-virtual read functions. This means if we - * want to "redefine" a field in a child class by calling ``DEFINE_FIELD`` again, then - * the function will be "hidden" instead of "override". This is important to remember - * when casting a pointer to a base type, as in this case the implementation from the - * base type will be used since the function created by ``DEFINE_FIELD`` is not virtual. + * ``DEFINE_FIELD`` and ``DEFINE_REGISTERED_FIELD`` create templated, non-virtual read + * functions. This means if we want to "redefine" a field in a child class by calling + * ``DEFINE_FIELD`` again, then the function will be "hidden" instead of "override". + * This is important to remember when casting a pointer to a base type, as in this case + * the implementation from the base type will be used since the function created + * by ``DEFINE_FIELD`` is not virtual. * * * @subsection implement_registered_type_example Example: Implementing a new type @@ -174,7 +184,7 @@ * * \snippet tests/examples/test_RegisteredType_example.cpp example_RegisterType_full * - * @section use_the_define_field_macro How to Use the DEFINE_FIELD macro + * @section use_the_define_field_macro How to use the DEFINE_FIELD macro * * The \ref DEFINE_FIELD macro takes the following main inputs: * @@ -211,5 +221,24 @@ * \ref AQNWB::NWB::TimeSeries::readData "TimeSeries::readData" ) * for reading data fields from a file. * + * @section use_the_define_registered_field_macro How to use the DEFINE_REGISTERED_FIELD macro + * + * The \ref DEFINE_REGISTERED_FIELD works much like the \ref DEFINE_FIELD macro macro but + * returns instances of specific subtypes of \ref AQNWB::NWB::RegisteredType "RegisteredType", + * rather than \ref AQNWB::IO::ReadDataWrapper "ReadDataWrapper". As such the main inputs for + * \ref DEFINE_REGISTERED_FIELD are as follows: + * + * * ``name``: The name of the function to generate. + * * ``registeredType`` : The specific subclass of \ref AQNWB::NWB::RegisteredType "RegisteredType" to use + * * ``fieldPath`` : Literal string with the relative path to the field within the schema of the + * respective neurodata_type. This is automatically being expanded at runtime to the full path. + * * ``description`` : Description of the field to include in the docstring for the docs + * + * All of these inputs are required. A typical example will look as follows: + * + * @code + * DEFINE_REGISTERED_FIELD(getData, DynamicTable, "my_table", My data table) + * @endcode + * * */ \ No newline at end of file diff --git a/docs/pages/userdocs/read.dox b/docs/pages/userdocs/read.dox index 38156850..fe36aac2 100644 --- a/docs/pages/userdocs/read.dox +++ b/docs/pages/userdocs/read.dox @@ -477,9 +477,22 @@ * * \snippet tests/examples/test_ecephys_data_read.cpp example_read_new_io_snippet * - * \subsubsection read_design_example_read_posthoc_search Searching for Registered Type Objects (e.g.,ElectricalSeries) + * \subsubsection read_predefined_registered_field Reading known RegisteredType object * - * Using the \ref AQNWB::IO::BaseIO::findTypes "findType()" function of our I/O object we can + * When the path and type of objects is fixed in the schema (or we know them based on other conventions), + * then we can read the types directly from the file. E.g., here we first read the + * \ref AQNWB::NWB::NWBFile "NWBFile" directly, which we know exists at the root "/" + * of the file. We then read the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable" + * via the predefined \ref AQNWB::NWB::NWBFile::readElectrodeTable "NWBFile::readElectrodeTable" + * method. The advantage of this approach is that we do not need to manually specify paths + * or object types. Similarlry, when we read the `locations` columns, we do not need to + * specify the name or the data type to use. + * + * \snippet tests/examples/test_ecephys_data_read.cpp example_read_predefined_types + * + * \subsubsection read_design_example_read_posthoc_search Searching for Registered Type objects (e.g.,ElectricalSeries) + * + * When paths are not fixed, we can use the \ref AQNWB::IO::BaseIO::findTypes "findType()" function of our I/O object to * conveniently search for objects with a given type. * * \snippet tests/examples/test_ecephys_data_read.cpp example_search_types_snippet @@ -496,7 +509,7 @@ * \warning * The current implementation of \ref AQNWB::IO::BaseIO::findTypes "findType()" is * not aware of inheritance but searches for exact matches of types only. - * However, we can search for objects of multiple different times at the same by + * However, we can search for objects of multiple different times at the same time by * specifying multiple types to search for in our ``typesToSearch``. * * The returned ``std::unordered_map`` uses the full to object as key and the full diff --git a/src/nwb/NWBFile.hpp b/src/nwb/NWBFile.hpp index a9e014ce..fc4ee856 100644 --- a/src/nwb/NWBFile.hpp +++ b/src/nwb/NWBFile.hpp @@ -131,6 +131,48 @@ class NWBFile : public Container RecordingContainers* recordingContainers = nullptr, std::vector& containerIndexes = emptyContainerIndexes); + DEFINE_REGISTERED_FIELD(readElectrodeTable, + ElectrodeTable, + ElectrodeTable::electrodeTablePath, + "table with the extracellular electrodes") + + DEFINE_FIELD(readNWBVersion, + AttributeField, + std::string, + "nwb_version", + File version string) + + DEFINE_FIELD(readFileCreateDate, + DatasetField, + std::any, + "file_create_date", + A record of the date the file was created and of subsequent + modifications) + + DEFINE_FIELD(readIdentifier, + DatasetField, + std::string, + "identifier", + A unique text identifier for the file) + + DEFINE_FIELD(readSessionDescription, + DatasetField, + std::string, + "session_description", + A description of the experimental session and data in the file) + + DEFINE_FIELD(readSessionStartTime, + DatasetField, + std::any, + "session_start_time", + Date and time of the experiment or session start) + + DEFINE_FIELD(readTimestampsReferenceTime, + DatasetField, + std::any, + "timestamps_reference_time", + Date and time corresponding to time zero of all timestamps) + protected: /** * @brief Creates the default file structure. @@ -181,43 +223,6 @@ class NWBFile : public Container inline const static std::string acquisitionPath = "/acquisition"; static std::vector emptyContainerIndexes; - DEFINE_FIELD(readNWBVersion, - AttributeField, - std::string, - "nwb_version", - File version string) - - DEFINE_FIELD(readFileCreateDate, - DatasetField, - std::any, - "file_create_date", - A record of the date the file was created and of subsequent - modifications) - - DEFINE_FIELD(readIdentifier, - DatasetField, - std::string, - "identifier", - A unique text identifier for the file) - - DEFINE_FIELD(readSessionDescription, - DatasetField, - std::string, - "session_description", - A description of the experimental session and data in the file) - - DEFINE_FIELD(readSessionStartTime, - DatasetField, - std::any, - "session_start_time", - Date and time of the experiment or session start) - - DEFINE_FIELD(readTimestampsReferenceTime, - DatasetField, - std::any, - "timestamps_reference_time", - Date and time corresponding to time zero of all timestamps) - private: /** * @brief The ElectrodeTable for the file diff --git a/src/nwb/hdmf/table/DynamicTable.hpp b/src/nwb/hdmf/table/DynamicTable.hpp index 25cc13d4..50c90109 100644 --- a/src/nwb/hdmf/table/DynamicTable.hpp +++ b/src/nwb/hdmf/table/DynamicTable.hpp @@ -87,7 +87,7 @@ class DynamicTable : public Container /** * @brief Read an arbitrary column of the DyanmicTable * - * For columns defined in the schema the correspoding DEFINE_REGISTERED_FIELD + * For columns defined in the schema the corresponding DEFINE_REGISTERED_FIELD * read functions are preferred because they help avoid the need for * specifying the specific name of the column and data type to use. * diff --git a/tests/examples/test_ecephys_data_read.cpp b/tests/examples/test_ecephys_data_read.cpp index ab791973..0556e85c 100644 --- a/tests/examples/test_ecephys_data_read.cpp +++ b/tests/examples/test_ecephys_data_read.cpp @@ -203,6 +203,22 @@ TEST_CASE("ElectricalSeriesReadExample", "[ecephys]") readio->open(FileMode::ReadOnly); // [example_read_new_io_snippet] + // [example_read_predefined_types] + // Read the NWBFile + auto readNWBFile = + NWB::RegisteredType::create("/", readio); + // Read the ElectrodesTable + auto readElectrodeTable = readNWBFile->readElectrodeTable(); + // read the location data. Note that both the type of the class and + // the data values is being set for us, here, VectorData + auto locationColumn = readElectrodeTable->readLocationColumn(); + auto locationColumnValues = locationColumn->readData()->values(); + // confirm that the values are correct + std::vector expectedLocationValues = { + "unknown", "unknown", "unknown", "unknown"}; + REQUIRE(locationColumnValues.data == expectedLocationValues); + // [example_read_predefined_types] + std::cout << "Searching and reading the ElectricalSeries container" << std::endl; // [example_search_types_snippet] @@ -280,9 +296,6 @@ TEST_CASE("ElectricalSeriesReadExample", "[ecephys]") // [example_read_generic_dataset_field_snippet] // [example_read_generic_registeredtype_field_snippet] - // read the NWBFile - auto readNWBFile = - NWB::RegisteredType::create("/", readio); // read the ElectricalSeries from the NWBFile object via the readField // method returning a generic std::shared_ptr auto readRegisteredType = readNWBFile->readField(esdata_path); From 48f2c8af3fea71d9775f2e64b5e311f83e8aef64 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 23:23:55 -0800 Subject: [PATCH 11/19] Add error check in DEFINE_REGISTERED_FIELD --- src/nwb/RegisteredType.hpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/nwb/RegisteredType.hpp b/src/nwb/RegisteredType.hpp index 8d04beed..1459e07d 100644 --- a/src/nwb/RegisteredType.hpp +++ b/src/nwb/RegisteredType.hpp @@ -400,15 +400,18 @@ class RegisteredType * types, e.g,. VectorData a user may want to change this to a \ * more specific subtype to use, e.g., VectorData \ * @return A shared pointer to an instance of ##registeredType representing \ - * the object \ + * the object. May return nullptr if the path does not exist \ * \ * description \ */ \ template \ inline std::shared_ptr name() const \ { \ - return RegisteredType::create(AQNWB::mergePaths(m_path, fieldPath), \ - m_io); \ + std::string objectPath = AQNWB::mergePaths(m_path, fieldPath); \ + if (m_io->objectExists(objectPath)) { \ + return RegisteredType::create(objectPath, m_io); \ + } \ + return nullptr; \ } } // namespace NWB From 4014b6179b0f199bfedd419725e1a228e826a7c6 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Sat, 18 Jan 2025 23:51:34 -0800 Subject: [PATCH 12/19] Removed unused code --- src/nwb/hdmf/table/DynamicTable.cpp | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/nwb/hdmf/table/DynamicTable.cpp b/src/nwb/hdmf/table/DynamicTable.cpp index 5bb8c1da..799dd0ca 100644 --- a/src/nwb/hdmf/table/DynamicTable.cpp +++ b/src/nwb/hdmf/table/DynamicTable.cpp @@ -80,16 +80,3 @@ void DynamicTable::addReferenceColumn(const std::string& name, colDescription); } } - -/*template -std::shared_ptr> DynamicTable::readColumn( - const std::string& colName) -{ - std::string columnPath = AQNWB::mergePaths(m_path, colName); - if (m_io->objectExists(columnPath)) { - if (m_io->getStorageObjectType(columnPath) == StorageObjectType::Dataset) { - return std::make_shared>(columnPath, m_io); - } - } - return nullptr; -}*/ \ No newline at end of file From 5100ad21a1ef15cfe90b8d179c3ac2b450fe83c1 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Wed, 22 Jan 2025 16:43:09 -0800 Subject: [PATCH 13/19] Update docs/pages/userdocs/read.dox Co-authored-by: Steph Prince <40640337+stephprince@users.noreply.github.com> --- docs/pages/userdocs/read.dox | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/pages/userdocs/read.dox b/docs/pages/userdocs/read.dox index 38156850..10f27958 100644 --- a/docs/pages/userdocs/read.dox +++ b/docs/pages/userdocs/read.dox @@ -308,7 +308,7 @@ * - \ref AQNWB::IO::BaseIO "BaseIO", \ref AQNWB::IO::HDF5::HDF5IO "HDF5IO" are responsible for * i) reading type attribute and group information, ii) searching the file for typed objects via * \ref AQNWB::IO::BaseIO::findTypes "findTypes()" methods, and iii) retrieving the paths of all - * object associated with a storage objects (e.g., a Group) via \ref AQNWB::IO::BaseIO::getStorageObjects "getStoragebjects()" + * object associated with a storage object (e.g., a Group) via \ref AQNWB::IO::BaseIO::getStorageObjects "getStorageObjects()" * * \subsubsection read_design_wrapper_registeredType RegisteredType * From 67bc2cd483862f15ead95bbad59409c923cee891 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Wed, 22 Jan 2025 16:43:25 -0800 Subject: [PATCH 14/19] Update src/Types.hpp Co-authored-by: Steph Prince <40640337+stephprince@users.noreply.github.com> --- src/Types.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Types.hpp b/src/Types.hpp index 17ebbdfc..6d45f767 100644 --- a/src/Types.hpp +++ b/src/Types.hpp @@ -39,7 +39,7 @@ class Types * \brief Helper struct to check if a value is a data field, i.e., * Dataset or Attribute * - * This function is used to enforce constrains on templated functions that + * This function is used to enforce constraints on templated functions that * should only be callable for valid StorageObjectType values */ template From 19fecb4225c58b70e8448124878a537fd8b72d92 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Wed, 22 Jan 2025 23:30:16 -0800 Subject: [PATCH 15/19] Add additional test case for HDF5IO::getStorageObjects --- tests/testHDF5IO.cpp | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/testHDF5IO.cpp b/tests/testHDF5IO.cpp index 72aacf11..f50831f9 100644 --- a/tests/testHDF5IO.cpp +++ b/tests/testHDF5IO.cpp @@ -142,6 +142,38 @@ TEST_CASE("getStorageObjects", "[hdf5io]") REQUIRE(groupContent.size() == 0); } + SECTION("attribute") + { + int attrData1 = 42; + hdf5io.createAttribute(BaseDataType::I32, &attrData1, "/", "attr1"); + auto attributeContent = hdf5io.getStorageObjects("/attr1"); + REQUIRE(attributeContent.size() == 0); + } + + SECTION("dataset w/o attribute") + { + // Dataset without attributes + hdf5io.createArrayDataSet( + BaseDataType::I32, SizeArray {0}, SizeArray {1}, "/data"); + auto datasetContent = hdf5io.getStorageObjects("/data"); + REQUIRE(datasetContent.size() == 0); + + // Dataset with attribute + int attrData1 = 42; + hdf5io.createAttribute(BaseDataType::I32, &attrData1, "/data", "attr1"); + auto dataContent2 = hdf5io.getStorageObjects("/data"); + REQUIRE(dataContent2.size() == 1); + REQUIRE( + dataContent2[0] + == std::make_pair(std::string("attr1"), StorageObjectType::Attribute)); + } + + SECTION("invalid path") + { + auto invalidPathContent = hdf5io.getStorageObjects("/invalid/path"); + REQUIRE(invalidPathContent.size() == 0); + } + SECTION("group with datasets, subgroups, and attributes") { hdf5io.createGroup("/data"); From 0cb0575eedc9d660b7ee4c4091bf6bc17afec5e0 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 23 Jan 2025 02:04:39 -0800 Subject: [PATCH 16/19] Fix linter and compiler warnings --- src/io/hdf5/HDF5IO.cpp | 8 ++++---- src/nwb/NWBFile.hpp | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/io/hdf5/HDF5IO.cpp b/src/io/hdf5/HDF5IO.cpp index 2313b05f..efe51b00 100644 --- a/src/io/hdf5/HDF5IO.cpp +++ b/src/io/hdf5/HDF5IO.cpp @@ -1025,8 +1025,8 @@ HDF5IO::getStorageObjects(const std::string& path, if (objectType == StorageObjectType::Attribute || objectType == StorageObjectType::Undefined) { - SizeType numAttrs = group.getNumAttrs(); - for (SizeType i = 0; i < numAttrs; ++i) { + unsigned int numAttrs = static_cast(group.getNumAttrs()); + for (unsigned int i = 0; i < numAttrs; ++i) { H5::Attribute attr = group.openAttribute(i); objects.emplace_back(attr.getName(), StorageObjectType::Attribute); } @@ -1036,8 +1036,8 @@ HDF5IO::getStorageObjects(const std::string& path, || objectType == StorageObjectType::Undefined) { H5::DataSet dataset = m_file->openDataSet(path); - SizeType numAttrs = dataset.getNumAttrs(); - for (SizeType i = 0; i < numAttrs; ++i) { + unsigned int numAttrs = static_cast(dataset.getNumAttrs()); + for (unsigned int i = 0; i < numAttrs; ++i) { H5::Attribute attr = dataset.openAttribute(i); objects.emplace_back(attr.getName(), StorageObjectType::Attribute); } diff --git a/src/nwb/NWBFile.hpp b/src/nwb/NWBFile.hpp index 0f900f29..5576cf84 100644 --- a/src/nwb/NWBFile.hpp +++ b/src/nwb/NWBFile.hpp @@ -130,7 +130,7 @@ class NWBFile : public Container const IO::BaseDataType& dataType = IO::BaseDataType::I16, RecordingContainers* recordingContainers = nullptr, std::vector& containerIndexes = emptyContainerIndexes); - + /** @brief Create AnnotationSeries objects to record data into. * Created objects are stored in recordingContainers. * @param recordingNames vector indicating the names of the AnnotationSeries From b0be5ef8a405b7fb426f7266b9d774a5e19cb5a9 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Thu, 23 Jan 2025 11:19:45 -0800 Subject: [PATCH 17/19] Fix build error after merge conflict --- tests/examples/test_ecephys_data_read.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/tests/examples/test_ecephys_data_read.cpp b/tests/examples/test_ecephys_data_read.cpp index 3db5a67d..0556e85c 100644 --- a/tests/examples/test_ecephys_data_read.cpp +++ b/tests/examples/test_ecephys_data_read.cpp @@ -296,9 +296,6 @@ TEST_CASE("ElectricalSeriesReadExample", "[ecephys]") // [example_read_generic_dataset_field_snippet] // [example_read_generic_registeredtype_field_snippet] - // read the NWBFile - auto readNWBFile = - NWB::RegisteredType::create("/", readio); // read the ElectricalSeries from the NWBFile object via the readField // method returning a generic std::shared_ptr auto readRegisteredType = readNWBFile->readField(esdata_path); From c24398de7c92271f64bb9d949e0ebb6e64dcd921 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Mon, 27 Jan 2025 14:57:26 -0800 Subject: [PATCH 18/19] Improve develeoper docs for REGISTER_SUBCLASS_WITH_TYPENAME --- docs/pages/devdocs/registered_types.dox | 72 ++++++++++++++++++++----- src/nwb/misc/AnnotationSeries.hpp | 1 + 2 files changed, 59 insertions(+), 14 deletions(-) diff --git a/docs/pages/devdocs/registered_types.dox b/docs/pages/devdocs/registered_types.dox index 1f01bc6d..89260a20 100644 --- a/docs/pages/devdocs/registered_types.dox +++ b/docs/pages/devdocs/registered_types.dox @@ -71,22 +71,12 @@ * the name of the namespace in the schema (e.g., "core", "hdmf-common"). In this way * we can look up the corresponding class for an object in a file based on the * ``neurodata_type`` and ``namespace`` attributes stored in the file. - * - * \note * A special version of the ``REGISTER_SUBCLASS`` macro, called ``REGISTER_SUBCLASS_WITH_TYPENAME``, * allows setting the typename explicitly as a third argument. This is for the **special case** - * where we want to implement a class for a modified type that does not have its - * own `neurodata_type` in the NWB schema. An example is `ElectrodesTable` in NWB ` will be used on read. To support reading using the + * more specific types, we can use the \ref DEFINE_REGISTERED_FIELD macro to define + * read methods that will return the approbriate type, e.g.: + * + * \code + * DEFINE_REGISTERED_FIELD(readElectrodeTable, + * ElectrodeTable, + * ElectrodeTable::electrodeTablePath, + * "table with the extracellular electrodes") + * \endcode + * + * in \ref AQNWB::NWB::NWBFile "NWBFile" to read the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable", + * or + * + * \code + * DEFINE_REGISTERED_FIELD( + * readGroupNameColumn, + * VectorData, + * "group_name", + * "the name of the ElectrodeGroup this electrode is a part of") + * \endcode + * + * in the \ref AQNWB::NWB::ElectrodeTable "ElectrodeTable" to read the `group_name` column + * as `VectorData` with the data type already specified as `std::string` at compile time. + * * */ \ No newline at end of file diff --git a/src/nwb/misc/AnnotationSeries.hpp b/src/nwb/misc/AnnotationSeries.hpp index a9ac2ac1..54fa28b1 100644 --- a/src/nwb/misc/AnnotationSeries.hpp +++ b/src/nwb/misc/AnnotationSeries.hpp @@ -34,6 +34,7 @@ class AnnotationSeries : public TimeSeries /** * @brief Initializes the AnnotationSeries * @param description The description of the AnnotationSeries. + * @param comments Comments about the AnnotationSeries. * @param dsetSize Initial size of the main dataset. This must be a vector * with one element specifying the length in time. * @param chunkSize Chunk size to use. From 82c943194ce8df26cb7e4e0ccaa919501f54078f Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Tue, 11 Feb 2025 14:21:41 -0800 Subject: [PATCH 19/19] Clarify comments for templates --- src/nwb/hdmf/base/Data.cpp | 11 ++++++++--- src/nwb/hdmf/base/Data.hpp | 5 ++++- src/nwb/hdmf/table/VectorData.cpp | 10 +++++++--- src/nwb/hdmf/table/VectorData.hpp | 5 ++++- 4 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/nwb/hdmf/base/Data.cpp b/src/nwb/hdmf/base/Data.cpp index 27f1cbe4..bd74edc2 100644 --- a/src/nwb/hdmf/base/Data.cpp +++ b/src/nwb/hdmf/base/Data.cpp @@ -2,12 +2,17 @@ namespace AQNWB::NWB { -// Register the base Data type (with default std::any) for use with -// RegisteredType::create + +// Explicitly register the Data specialization with the type registry. +// This ensures that the most generic specialization is available for dynamic +// creation with RegisteredType::create. The REGISTER_SUBCLASS_IMPL macro +// initializes the static member to trigger the registration. template<> REGISTER_SUBCLASS_IMPL(Data) -// Explicitly instantiate for all common data types +// Explicitly instantiate the Data template for all common data types. +// This ensures that these specializations are generated by the compiler, +// reducing compile times and ensuring availability throughout the program. template class Data; template class Data; template class Data; diff --git a/src/nwb/hdmf/base/Data.hpp b/src/nwb/hdmf/base/Data.hpp index 727da973..96d0e478 100644 --- a/src/nwb/hdmf/base/Data.hpp +++ b/src/nwb/hdmf/base/Data.hpp @@ -15,7 +15,10 @@ template class Data : public RegisteredType { public: - // Register Data class as a registered type + // Register the Data template class with the type registry for dynamic + // creation. This registration is generic and applies to any specialization of + // the Data template. It allows the system to dynamically create instances of + // Data with different data types. REGISTER_SUBCLASS_WITH_TYPENAME(Data, "hdmf-common", "Data") /** diff --git a/src/nwb/hdmf/table/VectorData.cpp b/src/nwb/hdmf/table/VectorData.cpp index 2740ba82..6bebf610 100644 --- a/src/nwb/hdmf/table/VectorData.cpp +++ b/src/nwb/hdmf/table/VectorData.cpp @@ -2,12 +2,16 @@ namespace AQNWB::NWB { -// Register the base VectorData type (with default std::any) for use with -// RegisteredType::create +// Explicitly register the Data specialization with the type registry. +// This ensures that the most generic specialization is available for dynamic +// creation with RegisteredType::create. The REGISTER_SUBCLASS_IMPL macro +// initializes the static member to trigger the registration. template<> REGISTER_SUBCLASS_IMPL(VectorData) -// Explicitly instantiate for all common data types +// Explicitly instantiate the Data template for all common data types. +// This ensures that these specializations are generated by the compiler, +// reducing compile times and ensuring availability throughout the program. template class VectorData; template class VectorData; template class VectorData; diff --git a/src/nwb/hdmf/table/VectorData.hpp b/src/nwb/hdmf/table/VectorData.hpp index dfc4faa0..482cfc56 100644 --- a/src/nwb/hdmf/table/VectorData.hpp +++ b/src/nwb/hdmf/table/VectorData.hpp @@ -27,7 +27,10 @@ class VectorData : public Data { } - // Register VectorData class as a registered type + // Register the Data template class with the type registry for dynamic + // creation. This registration is generic and applies to any specialization of + // the Data template. It allows the system to dynamically create instances of + // Data with different data types. REGISTER_SUBCLASS_WITH_TYPENAME(VectorData, "hdmf-common", "VectorData")