From ae989a346e5691acf2096179db4655c784f887f1 Mon Sep 17 00:00:00 2001 From: Oliver Ruebel Date: Fri, 21 Feb 2025 19:53:50 -0800 Subject: [PATCH] Add support for reading reference attributes (#158) * change electrodeInd vector data type * define fields for electrodes * add tests for electrodes dataset in electricalseries data write * Support reading of reference attributes * Support reading of attribute references to registered types * Support reading of the electrodes/table attribute in ElectricalSeries * Use std::vector instead of char array in readReferenceAttribute * Minor doc fixes in ElectricalSeries * Minor doc fixes in ElectricalSeries * Revert renaming of readElectrodes * Revert renaming of readElectrodes * Remove unused variable from unit test * Update docs/pages/devdocs/registered_types.dox Co-authored-by: Steph Prince <40640337+stephprince@users.noreply.github.com> * Update src/nwb/RegisteredType.hpp Co-authored-by: Steph Prince <40640337+stephprince@users.noreply.github.com> * Update src/nwb/ecephys/ElectricalSeries.hpp Co-authored-by: Steph Prince <40640337+stephprince@users.noreply.github.com> * Fix linter error * Add unit test to read a non-reference attribute as a reference attribute * Added comment and exclude from codecov for extra runtime error checks * Add TODO note for deprecated function --------- Co-authored-by: Steph Prince <40640337+stephprince@users.noreply.github.com> --- docs/Doxyfile.in | 1 + docs/pages/devdocs/registered_types.dox | 12 ++++- src/io/BaseIO.hpp | 9 ++++ src/io/hdf5/HDF5IO.cpp | 63 +++++++++++++++++++++++++ src/io/hdf5/HDF5IO.hpp | 9 ++++ src/nwb/RegisteredType.hpp | 50 ++++++++++++++++++++ src/nwb/ecephys/ElectricalSeries.hpp | 20 ++++++-- tests/testEcephys.cpp | 9 ++-- tests/testHDF5IO.cpp | 50 +++++++++++++++++++- 9 files changed, 211 insertions(+), 12 deletions(-) diff --git a/docs/Doxyfile.in b/docs/Doxyfile.in index b8dd029f..88cc9ec1 100644 --- a/docs/Doxyfile.in +++ b/docs/Doxyfile.in @@ -17,6 +17,7 @@ PROJECT_NUMBER = "@PROJECT_VERSION@" 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 */ template inline std::shared_ptr name() const;" +PREDEFINED += "DEFINE_REFERENCED_REGISTERED_FIELD(name, registeredType, fieldPath, description)=/** description */ template inline std::shared_ptr name() const;" EXPAND_ONLY_PREDEF = YES diff --git a/docs/pages/devdocs/registered_types.dox b/docs/pages/devdocs/registered_types.dox index 89260a20..ef575248 100644 --- a/docs/pages/devdocs/registered_types.dox +++ b/docs/pages/devdocs/registered_types.dox @@ -213,7 +213,7 @@ * * @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 + * The \ref DEFINE_REGISTERED_FIELD macro 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: @@ -230,6 +230,16 @@ * DEFINE_REGISTERED_FIELD(getData, DynamicTable, "my_table", My data table) * @endcode * + * @section use_the_define_referenced_registered_field_macro How to use the DEFINE_REFERENCED_REGISTERED_FIELD macro + * + * The \ref DEFINE_REFERENCED_REGISTERED_FIELD macro works exactly like the + * \ref DEFINE_REGISTERED_FIELD macro, but the underlying data is an attribute that + * stores a reference to an instances of a specific subtype of \ref AQNWB::NWB::RegisteredType "RegisteredType" + * rather than the instance of the object directly. I.e., ``fieldPath`` here is the + * relative path to the attribute that stores the reference, rather than the relative path + * of the object itself. The generated read method then resolves the reference first and + * then returns the instance of the object that is being referenced. + * * @section using_registered_subclass_with_typename Using REGISTER_SUBCLASS_WITH_TYPENAME * * The main use case for \ref REGISTER_SUBCLASS_WITH_TYPENAME is when we need to implement diff --git a/src/io/BaseIO.hpp b/src/io/BaseIO.hpp index 7c506b4a..a7baa062 100644 --- a/src/io/BaseIO.hpp +++ b/src/io/BaseIO.hpp @@ -298,6 +298,15 @@ class BaseIO */ virtual DataBlockGeneric readAttribute(const std::string& dataPath) const = 0; + /** + * @brief Reads a reference attribute and returns the path to the referenced + * object. + * @param dataPath The path to the reference attribute within the file. + * @return The path to the referenced object. + */ + virtual std::string readReferenceAttribute( + const std::string& dataPath) const = 0; + /** * @brief Creates an attribute at a given location in the file. * @param type The base data type of the attribute. diff --git a/src/io/hdf5/HDF5IO.cpp b/src/io/hdf5/HDF5IO.cpp index e523f3f4..35f7c7bd 100644 --- a/src/io/hdf5/HDF5IO.cpp +++ b/src/io/hdf5/HDF5IO.cpp @@ -301,6 +301,69 @@ std::vector HDF5IO::readStringDataHelper( dataSource, numElements, H5::DataSpace(), H5::DataSpace()); } +std::string HDF5IO::readReferenceAttribute(const std::string& dataPath) const +{ + // Read the attribute + auto attributePtr = this->getAttribute(dataPath); + if (attributePtr == nullptr) { + throw std::invalid_argument( + "HDF5IO::readReferenceAttribute, attribute does not exist."); + } + + H5::Attribute& attribute = *attributePtr; + H5::DataType dataType = attribute.getDataType(); + + // Check if the attribute is a reference + if (dataType != H5::PredType::STD_REF_OBJ) { + throw std::invalid_argument( + "HDF5IO::readReferenceAttribute, attribute is not a reference."); + } + + // Read the reference + hobj_ref_t ref; + attribute.read(dataType, &ref); + + // Dereference the reference to get the HDF5 object ID + // TODO: Note as of HDF5-1.12, H5Rdereference2() has been deprecated in + // favor of H5Ropen_attr(), H5Ropen_object() and H5Ropen_region(). + hid_t obj_id = + H5Rdereference2(attribute.getId(), H5P_DEFAULT, H5R_OBJECT, &ref); + if (obj_id < 0) { + throw std::runtime_error( + "HDF5IO::readReferenceAttribute, failed to dereference object."); + } + + // Get the name (path) of the dereferenced object + ssize_t buf_size = H5Iget_name(obj_id, nullptr, 0) + 1; + // LCOV_EXCL_START + // This is a safety check to safeguard against possible runtime issues, + // but this should never happen. + if (buf_size <= 0) { + H5Oclose(obj_id); + throw std::runtime_error( + "HDF5IO::readReferenceAttribute, failed to get object name size."); + } + // LCOV_EXCL_STOP + + std::vector obj_name(static_cast(buf_size)); + // LCOV_EXCL_START + // This is a safety check to safeguard against possible runtime issues, + // but this should never happen. + if (H5Iget_name(obj_id, obj_name.data(), static_cast(buf_size)) < 0) { + H5Oclose(obj_id); + throw std::runtime_error( + "HDF5IO::readReferenceAttribute, failed to get object name."); + } + // LCOV_EXCL_STOP + + std::string referencedPath(obj_name.data()); + + // Close the dereferenced object + H5Oclose(obj_id); + + return referencedPath; +} + AQNWB::IO::DataBlockGeneric HDF5IO::readAttribute( const std::string& dataPath) const { diff --git a/src/io/hdf5/HDF5IO.hpp b/src/io/hdf5/HDF5IO.hpp index d89872fc..eeaee0e0 100644 --- a/src/io/hdf5/HDF5IO.hpp +++ b/src/io/hdf5/HDF5IO.hpp @@ -125,6 +125,15 @@ class HDF5IO : public BaseIO AQNWB::IO::DataBlockGeneric readAttribute( const std::string& dataPath) const override; + /** + * @brief Reads a reference attribute and returns the path to the referenced + * object. + * @param dataPath The path to the reference attribute within the file. + * @return The path to the referenced object. + */ + std::string readReferenceAttribute( + const std::string& dataPath) const override; + /** * @brief Creates an attribute at a given location in the file. * @param type The base data type of the attribute. diff --git a/src/nwb/RegisteredType.hpp b/src/nwb/RegisteredType.hpp index e8a86a45..eec83051 100644 --- a/src/nwb/RegisteredType.hpp +++ b/src/nwb/RegisteredType.hpp @@ -427,5 +427,55 @@ class RegisteredType return nullptr; \ } +/** + * @brief Defines a lazy-loaded accessor function for reading fields that are + * RegisteredTypes that are linked to by a reference attribute + * + * This macro generates a function that returns the appropriate subtype of + * RegisteredType, e.g., to read VectorData from a DynamicTable or a + * TimeSeries from an 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 attribute that stores reference to the field + * @param description A detailed description of the field. + */ +#define DEFINE_REFERENCED_REGISTERED_FIELD( \ + name, registeredType, fieldPath, description) \ + /** \ + * @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. May return nullptr if the path does not exist \ + * \ + * description \ + */ \ + template \ + inline std::shared_ptr name() const \ + { \ + try { \ + std::string attrPath = AQNWB::mergePaths(m_path, fieldPath); \ + std::string objectPath = m_io->readReferenceAttribute(attrPath); \ + if (m_io->objectExists(objectPath)) { \ + return RegisteredType::create(objectPath, m_io); \ + } \ + } catch (const std::exception& e) { \ + return nullptr; \ + } \ + return nullptr; \ + } + } // namespace NWB } // namespace AQNWB diff --git a/src/nwb/ecephys/ElectricalSeries.hpp b/src/nwb/ecephys/ElectricalSeries.hpp index ca7d23fb..4665d226 100644 --- a/src/nwb/ecephys/ElectricalSeries.hpp +++ b/src/nwb/ecephys/ElectricalSeries.hpp @@ -7,6 +7,7 @@ #include "io/BaseIO.hpp" #include "io/ReadIO.hpp" #include "nwb/base/TimeSeries.hpp" +#include "nwb/file/ElectrodeTable.hpp" namespace AQNWB::NWB { @@ -109,17 +110,26 @@ class ElectricalSeries : public TimeSeries Base unit of measurement for working with the data. This value is fixed to volts) - DEFINE_FIELD(readElectrodes, - DatasetField, - int, - "electrodes", - The electrodes that generated this electrical series.) + DEFINE_FIELD( + readElectrodes, + DatasetField, + int, + "electrodes", + The indices of the electrodes that generated this electrical series.) DEFINE_FIELD(readElectrodesDescription, AttributeField, std::string, "electrodes/description", The electrodes that generated this electrical series.) + + DEFINE_REFERENCED_REGISTERED_FIELD( + readElectrodesTable, + ElectrodeTable, + "electrodes/table", + The electrodes table retrieved from the object referenced in the + `electrodes / table` attribute.) + private: /** * @brief The number of samples already written per channel. diff --git a/tests/testEcephys.cpp b/tests/testEcephys.cpp index b2c99174..69361d03 100644 --- a/tests/testEcephys.cpp +++ b/tests/testEcephys.cpp @@ -244,10 +244,11 @@ TEST_CASE("ElectricalSeries", "[ecephys]") REQUIRE(readElectrodesDescriptionValues == "the electrodes that generated this electrical series"); - // TODO - add test for reading when references are supported in read - // auto readElectrodesTableWrapper = - // readElectricalSeries->readElectrodesTable(); auto - // readElectrodesTableValues = readElectrodesTableWrapper->values().data[0]; + // Read the references to the ElectrodeTable + auto readElectrodesTable = readElectricalSeries->readElectrodesTable(); + REQUIRE(readElectrodesTable != nullptr); + REQUIRE(readElectrodesTable->getPath() + == AQNWB::NWB::ElectrodeTable::electrodeTablePath); } } diff --git a/tests/testHDF5IO.cpp b/tests/testHDF5IO.cpp index 10e72958..a946264b 100644 --- a/tests/testHDF5IO.cpp +++ b/tests/testHDF5IO.cpp @@ -636,10 +636,56 @@ TEST_CASE("HDF5IO; create attributes", "[hdf5io]") REQUIRE(hdf5io.objectExists("/data/link")); } - // reference + // reference attribute tests SECTION("reference") { - // TODO + // Create target objects that we'll reference + hdf5io.createGroup("/referenceTargetGroup"); + hdf5io.createArrayDataSet(BaseDataType::I32, + SizeArray {3}, + SizeArray {3}, + "/referenceTargetDataset"); + + // Test reference to a group + SECTION("reference to group") + { + hdf5io.createReferenceAttribute( + "/referenceTargetGroup", "/data", "groupRefAttr"); + REQUIRE(hdf5io.attributeExists("/data/groupRefAttr")); + + std::string resolvedPath = + hdf5io.readReferenceAttribute("/data/groupRefAttr"); + REQUIRE(resolvedPath == "/referenceTargetGroup"); + } + + // Test reference to a dataset + SECTION("reference to dataset") + { + hdf5io.createReferenceAttribute( + "/referenceTargetDataset", "/data", "datasetRefAttr"); + REQUIRE(hdf5io.attributeExists("/data/datasetRefAttr")); + + std::string resolvedPath = + hdf5io.readReferenceAttribute("/data/datasetRefAttr"); + REQUIRE(resolvedPath == "/referenceTargetDataset"); + } + + // Test reading non-existent reference attribute + SECTION("non-existent reference attribute") + { + REQUIRE_THROWS_AS( + hdf5io.readReferenceAttribute("/data/nonExistentRefAttr"), + std::invalid_argument); + } + + // Test reading an attribute that is not a reference attribute + SECTION("non-reference attribute") + { + const int data = 1; + hdf5io.createAttribute(BaseDataType::I32, &data, "/data", "intAttr"); + REQUIRE_THROWS_AS(hdf5io.readReferenceAttribute("/data/intAttr"), + std::invalid_argument); + } } // close file