Add HDF5IO tests for add_read PR

[oruebel]

This PR targets #85 to complete open ToDo items to add unit tests.

Add unit tests for public HDF5IO-specific functions (i.e., not inherited from BaseIO)

• HDF5IO::getH5ObjectType
• HDF5IO::getNativeType
• HDF5IO::getH5Type

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.
• HDF5IO::open(FileMode mode) test that file modes are being set correctly
• HDF5IO::readAttribute
• HDF5IO::getStorageObjectType
• HDF5IO::getGroupObjects
• HDF5IO::attributeExists
• HDF5IO::objectExists

New TODO items identified in unit tests:

• Update HDF5IO::readDataset to cover all numeric types in BaseDataType
• Debug reading of strings and update theTEST_CASE("HDF5IO; read dataset", "[hdf5io]") and TEST_CASE("HDF5IO; read dataset subset", "[hdf5io]").
  • Fixed writing of fixed length strings (the data type was not being set correctly)
  • Fixed reading of fixed length strings
  • Fixed writing of variable length strings via HDF5RecordingData::writeDataBlock
  • Fixed reading of variable length string datasets
  • Fixed reading of empty string datasets

[oruebel]

@stephprince there is still an issue with reading strings, but otherwise this PR is ready for you to take a look at.

[oruebel]

I have confirmed now that the fixed length string dataset is being written correctly. However, reading of fixed length string data still fails. I have not investigated the issues with variable length strings yet.

[oruebel]

Ok, fixed length strings seem to work now for both write and read. Next up, variable length strings.

[oruebel]

@stephprince I've now fixed the writing/reading of fixed and variable length strings. This PR is ready for you to take a look at. I think the other TODO items for #85 are probably best done in additional PRs, otherwise this PR will become to large.

The last remaining issue for this PR is to fix the address sanitizer test. It looks like the sanitizer doesn't like something about the changes in HDF5RecordingData::writeDataBlock . Getting string datasets to work for write/read is a bit tricky.

[stephprince]

looks good to me! thanks for adding all the tests

[stephprince]

Just to clarify, is this functionality needed in addition to the Status HDF5IO::createStringDataSet(const std::string& path, const std::vector<std::string>& values) function or intended to replace it?

[oruebel]

Good point. I think the key difference is that createStringDataSet is a one-shot write, i.e., it creates and writes the data in the same call. Using HDF5RecoringData however writes data in blocks, so you can create an array of strings and write them as you go. As such, createStringDataSet is nice for creating metadata datasets where the values are known, and HDF5RecordingData::writeStringDataBlock (combined with HDF5IO::createArrayDataSet) is useful when acquiring string data.

TBH, I was creating unit tests for all main data types to test reading and writing and simply tried to follow the same pattern for all data types, and didn't realize that string data was following a different pattern. That being said, I think it is useful to be able to acquire string data in blocks as well.

I guess the question is, whether we want to: a) always go the route of first calling create an array dataset with HDF5IO::createArrayDataset and then writing the data with BaseRecordingData::writeDataBlock calls or b) have additional functions on HDF5IO for one-shot writes where we create and write the dataset in the same call. I think in principle option b is fine, with the caveat that we have one-shot write functions only for a few special types like strings and references.

What do you think?

[stephprince]

As such, createStringDataSet is nice for creating metadata datasets where the values are known, and HDF5RecordingData::writeStringDataBlock (combined with HDF5IO::createArrayDataSet) is useful when acquiring string data.

Thanks for clarifying! That makes sense.

I guess the question is, whether we want to: a) always go the route of first calling create an array dataset with HDF5IO::createArrayDataset and then writing the data with BaseRecordingData::writeDataBlock calls or b) have additional functions on HDF5IO for one-shot writes where we create and write the dataset in the same call

I also think option B is fine and it is useful to have that functionality for small datasets that are known before the recording starts. I agree we should probably limit it to a few special types and in most cases follow the 1) create an array dataset with HDF5IO::createArrayDataset and 2) write the data with BaseRecordingData::writeDataBlock pattern.

If we go with option B, we may want to update createStringDataset to use writeStringDataBlock to make it consistent and easier to update string dataset writing in the future. Similarly for createReferenceDataset, though I think we do not have testing for writing datasets of references yet(?) so I'm not sure if that's fully supported or would also require a writeReferenceDataBlock method.

[oruebel]

I also think option B is fine

This sounds all good to me.

we may want to update createStringDataset to use writeStringDataBlock to make it consistent and easier to update string dataset writing in the future.

Makes sense

Similarly for createReferenceDataset, though I think we do not have testing for writing datasets of references yet(?) so I'm not sure if that's fully supported or would also require a writeReferenceDataBlock method.

I think you are correct that HDF5RecordingData::writeDataBlock probably doesn't support writing references yet and that we may need to handle that as a a separate HDF5RecordingData::writeReferenceDataBlock. I think we can make that a separate issue though.

[stephprince]

yes, definitely think we can handle writing references with writeDataBlock in a separate PR.

[oruebel]

The last remaining issue for this PR is to fix the address sanitizer test. It looks like the sanitizer doesn't like something about the changes in HDF5RecordingData::writeDataBlock . Getting string datasets to work for write/read is a bit tricky.

To get writing of string data to work cleanly, I added a new function HDF5RecordingData::writeStringDataBlock to avoid the error-prone casting of void* back to std::vector<std::string>. This has fixed the address sanitizer issues, so this PR is now good for final review.

@stephprince the change of adding HDF5RecordingData::writeStringDataBlock also relates directly to the question you raised above, i.e., do we want to support acquiring string data in blocks or is it ok to only support one-shot writing of strings via HDF5IO::createStringDataSet?

[stephprince]

I was reminded as I was working on a different PR that I still need to fix the nwbinspector validate workflow because it will currently pass even if there are critical errors (see #96).

I'll fix the workflow, but in the meantime just wanted to note that there seem to be some validation issues introduced in this PR (and the base read PR) even though the tests are passing.

[oruebel]

How do I need to run the NWB Inspector locally to see the validation errors?

[stephprince]

Run nwbinspector . --threshold BEST_PRACTICE_VIOLATION in the build folder containing the test data (e.g. build/dev/tests/data). It should inspect only the files ending in .nwb

[oruebel]

just wanted to note that there seem to be some validation issues introduced in this PR (and the base read PR) even though the tests are passing

I added this as a TODO item on #85 so we can address it in a separate PR. In the meantime I would like to merge this PR with #85.