Add data read

[oruebel]

This PR is to try and implement the proposed approach for data read from #83 to see if this approach is viable. This PR is experimental right now and should not be merged.

2. Proposed Implementation for reading data arrays

BaseReadData

• Create a new ReadDatasetWrapper and ReadAttributeWrapper classes for reading data. This is modified from the proposal, which suggested a single BaseReadData class for reading any array (datasets, attributes) from a file.
• Support conversion to boost multi-dimensional array for convenience
• Note I did not update BaseRecordingData to inherit from ReadDataWrapper because the two are not compatible right now. BaseReadData uses the io and the path to access the data, whereas BaseRecordingData leaves that choice to the I/O backend and stores the references to the dataset. We may or may not want to change this.

BaseIO

• Note I did not add abstract methods for lazy reading objects from a file to BaseIO (or more accurately I remove them) because: 1) I wanted to use the shared_ptr to the I/O rather than a raw pointer, which I can't get from BaseIO, and 2) with the ReadDatasetWrapper this is more approbriately done in the Container class directly.
• Add pure virtual method to allow us to get the storage object type (Group, Dataset, Attribute) for a given path
• Add pure virtual methods to read data values from a Dataset or Attribute that the BaseReadData can call for read

HDF5IO

• Note In contrast to the proposal, I did not implement specific version of the BaseReadData for HDF5 but left read logic to HDF5IO itself so that the ReadDatasetWrapper can remain generic. To make this more manageable, I defined ReadAttributeWrapper separately
• Implement the methods for reading data values from a Dataset or Attribute that the HDF5ReadDataSet and HDF5ReadAttribute wrappers can call can call for read
• Implement the getObjectType method for getting the storage object type (Group, Dataset, Attribute) for a given path

Container

• Store the io object on the Container so that we can call io->readDataset and io->readAttribute in the read methods

NWB types: TimeSeries, ElectricalSeries etc.

• Remove storage of properties from the Container classes and replace them with access methods that return BaseReadData objects instead. This allows for reading in both read and write mode and avoids keeping data in memory that we have already written to disk. For example, in TimeSeries, these variables would need to change to properties:

  aqnwb/src/nwb/base/TimeSeries.hpp

  Lines 91 to 140 in e873d95

  	/**
  	* @brief Base unit of measurement for working with the data. Actual stored
  	* values are not necessarily stored in these units. To access the data in
  	* these units, multiply ‘data’ by ‘conversion’ and add ‘offset’.
  	*/
  	std::string unit;
  	/**
  	* @brief The description of the TimeSeries.
  	*/
  	std::string description;
  	/**
  	* @brief Human-readable comments about the TimeSeries.
  	*/
  	std::string comments;
  	/**
  	* @brief Size used in dataset creation. Can be expanded when writing if
  	* needed.
  	*/
  	SizeArray dsetSize;
  	/**
  	* @brief Chunking size used in dataset creation.
  	*/
  	SizeArray chunkSize;
  	/**
  	* @brief Scalar to multiply each element in data to convert it to the
  	* specified ‘unit’.
  	*/
  	float conversion;
  	/**
  	* @brief Smallest meaningful difference between values in data, stored in the
  	* specified by unit.
  	*/
  	float resolution;
  	/**
  	* @brief Scalar to add to the data after scaling by ‘conversion’ to finalize
  	* its coercion to the specified ‘unit’.
  	*/
  	float offset;
  	/**
  	* @brief The starting time of the TimeSeries.
  	*/
  	float startingTime = 0.0;

• Add access methods that return BaseReadData for missing fields

3. Proposed implementation for reading whole Containers (e.g., to read an ElectricalSeries)

• Add access methods on the respective Container that owns the respective objects, e.g., NWBFile owning ElectricalSeries objects to retrieve the object
• Add abstract factory method (that is templated on the return type) to Container to create an instance of the specific Container type using only the io and path for the Container as input. The specific Container classes, such as TimeSeries will then need to implement a corresponding constructor that uses io and path as input.

Step 1: Define the Template Factory Method in Container

class Container {
public:
   
    template <typename T>
    static std::unique_ptr<T> create(const BaseIO& io, const std::string& path) {
        static_assert(std::is_base_of<Container, T>::value, "T must be a derived class of Container");
        return std::unique_ptr<T>(new T(path, io));
    }
};

Step 2: Implement the constructors on the specific Container classes (e.g., TimeSeries)

• Add the necessary constructor

class TimeSeries : public Container {
public:
    TimeSeries(const std::string& path, const BaseIO& io) {
        // Implementation of TimeSeries constructor
    }
};

4. Proposed implementation for reading untyped groups (e.g., /acquisition)

I'm not sure we'll need do this, since a group by itself does not define data. To access the contents we could define access methods on the parent Container class (e.g., NWBFile) that owns the untyped group to access its contents.

TODO

• Implement method to construct Container for read from NWBFile (See Add read for neurodata_types, e.g., Container, TimeSeries #91)
• Check all Container classes (e.g., ElectrodeGroup etc.) to make sure we move all the arguments (other than path and io) to the initialize method. Done in Add read for neurodata_types, e.g., Container, TimeSeries #91
• Check if we can fix data type to retrieve type ReadDatasetWrapper, ReadAttributeWrapper (See Add read for neurodata_types, e.g., Container, TimeSeries #91)
• Check how we can allow the user query the data type to use for an object (See Add read for neurodata_types, e.g., Container, TimeSeries #91)
• Clean up storage of properties that should be read (See Add read for neurodata_types, e.g., Container, TimeSeries #91)
• Debug reading of attributes
• Test reading of string datasets and attributes
• Add more detailed docs for reading
• Move code from functions defined in the header files (mainly for quick testing) to the cpp files
• Add common base class for ReadDatasetWrapper and ReadAttributeWrapper (I think this could be useful)
• Add support for storing the native data type on the DataBlockGeneric to make it easier for the user to inspect it
• Move BaseIO and HDF5 to a new IO module Fix Propose refactor of I/O class organization #88
• Refactor BaseIO to separate certain classes into their own hpp/cpp files

Items moved to new issues

• Support read with classes using REGISTER_SUBCLASS_WITH_TYPENAME #115
• Implement read for compound data types and references #114
• Add developer documentation note on merge commits for PRs #113

Next steps

• Check that workflows are running the correct code version. The build in the test fails with /Users/runner/work/aqnwb/aqnwb/src/nwb/ecephys/ElectricalSeries.cpp:61:25: error: use of undeclared identifier 'BaseDataType'; did you mean 'IO::BaseDataType'? m_io->createAttribute(BaseDataType::I32, But that is not what in line 61 in this PR https://github.com/NeurodataWithoutBorders/aqnwb/actions/runs/11023552242/job/30615439516?pr=85#step:6:249
• Once the workflow issue is resolved, I propose we merge Add read for neurodata_types, e.g., Container, TimeSeries #91 with this PR to create a single PR
• See Add read for neurodata_types, e.g., Container, TimeSeries #91 for open ToDo items that should be addressed before merge
• Add unit tests for read. For every neurodata_type we should test that: a) the type itself can be read from file, and b) that all DEFINE_FIELDS specified for read can be read. As part of writing the tests, we should also double-check for each type, that all fields that AqNWB writes, that they are also exposed for read.
  • TimeSeries See Add read tests for TimeSeries #124
  • ElectricalSeries
  • SpikeEventSeries
  • Device
  • NWBFile. NOTE: For reading the NWBFile container, there is the special case that NWBFile requires the path to the "/" so this may require some additional logic in the tests.

    aqnwb/src/nwb/NWBFile.cpp

    Lines 39 to 44 in bfa740a

    	NWBFile::NWBFile(const std::string& path, std::shared_ptr<IO::BaseIO> io)
    	: Container("/", io) // Always use "/" for the path
    	{
    	std::cerr << "NWBFile object is always the root. Path must be /" << std::endl;
    	assert(path == "/");
    	}

  • ElectrodeTable. NOTE: ElectrodeTable is currently not a neurodata_type and uses REGISTER_SUBCLASS_WITH_TYPENAME. I.e., on read this will return a DynamicTable instead of an ElectrodeTable right now. See Support read with classes using REGISTER_SUBCLASS_WITH_TYPENAME #115
  • ElectrodeGroup
  • DynamicTable
  • Data
  • VectorData
  • ElementIdentifiers
  • ...
• Add unit test for specific new features:
  • src/Utils.cpp , mergePaths() function
• Add unit test for BaseIO::findTypes (possibly as part of the HDF5IO tests)
• Merge PR Update read example #123 to update the read example
• Merge PR Add HDF5IO tests for add_read PR #118 with this PR
• Add unit tests for public HDF5IO-specific functions (i.e., not inherited from BaseIO:
  • HDF5IO::getH5ObjectType. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::getNativeType. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::getH5Type. Implemented in Add HDF5IO tests for add_read PR #118
• Add unit tests for new publicHDF5IO functions (which are core functions new inBaseIO). While some of these are partially covered by the DEFINE_FIELDS read tests, it will be useful to more fully test these separately
  • HDF5IO::readDataset (incl. with start, count, stride, and block parameters set). This is to test that reading subsets of larger arrays works as expected. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::open(FileMode mode) test that file modes are being set correctly. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::readAttribute. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::getStorageObjectType. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::getGroupObjects. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::attributeExists. Implemented in Add HDF5IO tests for add_read PR #118
  • HDF5IO::objectExists . Implemented in Add HDF5IO tests for add_read PR #118
• Update HDF5IO::readDataset to make sure it supports all supported types of BaseDataType, in partiuclar also strings and update theTEST_CASE("HDF5IO; read dataset", "[hdf5io]") and TEST_CASE("HDF5IO; read dataset subset", "[hdf5io]") (See Add HDF5IO tests for add_read PR #118 )
• Add unit tests for new private HDF5IO internal functions. These may not require separate dedicated tested, but may be covered via the tests of the public core interfaces functions. However, we should check that they are indeed covered:
  • HDF5IO:: readDataHelper (note, there are two variants of this function)
  • HDF5IO::readStringDataHelper (note there are two variants of this function)
  • HDF5IO::getAttribute
• Fix nwb-inspector violations. (cd build/dev/tests/data and run nwbinspector . --threshold BEST_PRACTICE_VIOLATION)
• Move colNames from DynamicTable constructor to initialize
• Check unit tests for RegisteredType. This should be largely covered by the other tests, but we should check.
• Check unit tests for ReadIO. This should be largely covered by the other tests, but we should check.
• Resolve open conversations:
  • Resolve comment on updating workflows to run all PRs https://github.com/NeurodataWithoutBorders/aqnwb/pull/85/files#r1779068283
  • Make DEFINE_FIELD create virtual functions and resolve this comment https://github.com/NeurodataWithoutBorders/aqnwb/pull/85/files#r1894223365 NOTE: I created Allow virtual function for DEFINE_FIELD #117 for this and added a note to the docs to example this. I think it is fine to address this later.
  • How to deal with ``DynamicTable.colNames` https://github.com/NeurodataWithoutBorders/aqnwb/pull/85/files#r1773941228
  • DynamicTable may require additional discussion on how to read columns. I think right now we would read individual VectorData columns manually
  • Fix failing Windows build due to 'boost/multi_array.hpp': No such file or directory

[oruebel]

@stephprince when you get a chance ,could you please do a first code review of this PR to make sure this is heading in the right direction. I now have a first outline of one possible solution for how we might implement read. There is still a lot more work to be done before this PR is ready, but it would be useful if you could take a look before I go any further with this approach.

I would start by looking at:

1. tests/examples/test_ecephys_data_read.cpp which shows an example of how read works for the user
2. BaseIO then defines the main new classes used for reading and HDF5IO then implements the actual reading
3. Container and ElectricalSeries also have some relevant changes to allow us to construct Container objects for read and how we can get specific datasets/attributes

[oruebel]

@stephprince I just added a documentation page as well, which hopefully helps explain the current proposed design for read so we can review and discuss.

[oruebel]

We could modify the tests.yml to run all the tests with both states, i.e., with the code as is in the PR as well as with the temporary merge. This would make the CI runtime longer (since all tests would run twice) and make the workflow a bit longer but would help with finding merge errors.

We decide not to do this and to continue testing only for the merged version. We decided to add a note in the developer docs to clarify this behavior.

[oruebel]

@stephprince I synced the branch with the main branch. However, Windows tests are currently failing due to 'boost/multi_array.hpp': No such file or directory. Can you check the Windows action to make sure boost is being installed correctly.

[oruebel]

, Windows tests are currently failing due to 'boost/multi_array.hpp': No such file or directory

Windows tests are working again. I had to add boost multi-array to the windows Action to fix the include error for boost/multi_array.hpp and update used of variable length arrays in HDF5IO to use std::vector instead, because apparently gcc has an extension to support variable length arrays but the default compiler on Windows does not.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 89.33518% with 231 lines in your changes missing coverage. Please review.

Project coverage is 90.87%. Comparing base (a532ec5) to head (44e957d).

Files with missing lines	Patch %	Lines
src/io/hdf5/HDF5IO.cpp	76.42%	169 Missing ⚠️
src/io/hdf5/HDF5RecordingData.cpp	73.87%	29 Missing ⚠️
src/io/ReadIO.hpp	79.48%	8 Missing ⚠️
src/nwb/RegisteredType.cpp	79.48%	8 Missing ⚠️
src/nwb/NWBFile.cpp	87.23%	6 Missing ⚠️
src/nwb/RegisteredType.hpp	55.55%	4 Missing ⚠️
src/io/BaseIO.cpp	95.77%	3 Missing ⚠️
src/Utils.hpp	90.47%	2 Missing ⚠️
src/nwb/file/ElectrodeTable.hpp	50.00%	1 Missing ⚠️
tests/examples/test_HDF5IO_examples.cpp	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #85      +/-   ##
==========================================
+ Coverage   90.32%   90.87%   +0.55%     
==========================================
  Files          39       53      +14     
  Lines        1830     3289    +1459     
  Branches      126      229     +103     
==========================================
+ Hits         1653     2989    +1336     
- Misses        167      290     +123     
  Partials       10       10              

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