Save electrode related information #1
Conversation
src/HDF5IO.hpp
Outdated
| /** Sets an object reference attribute for a given location in the file */ | ||
| int setAttributeRef(std::string referencePath, | ||
| std::string path, | ||
| std::string name) override; |
There was a problem hiding this comment.
The naming of setAttributeRef and createReferenceDatasSet seems inconsistent. I would suggest either:
createReferenceAttributeandcreateReferenceDatasetorcreateDataSetOfReferencesandcreateAttributeOfReferences
Since other functions are called ``createStringDataset` I guess option is probably more consistent.
If need to distinguish between Attributes that are scalars vs. arrays, we could add a bool scalar = True on the function or have separate functions for `createScalarAttributeandcreateArrayAttribute``
There was a problem hiding this comment.
Good point, I didn't realize the naming of those was inconsistent. I updated them to Option 1. I also changed it to createAttribute instead of setAttribute to be consistent.
For the scalars vs. arrays, right now I've overloaded the Attribute function to deal with arrays vs. scalars. Is using bool scalar = True preferable?
| protected: | ||
| /** | ||
| * @brief The current position in the x dimension. | ||
| */ | ||
| int xPos; |
There was a problem hiding this comment.
Does this mean BaseRecordingDatat assumes appending along the first dimension x (i.e., time in most cases in NWB)
There was a problem hiding this comment.
The equivalent RecordingData class in OpenEphys had methods for writing data along first dimension (time), 2D blocks (time x channels), and writing rows of data in a 2D block (time x channels). I hadn't added the 2D implementations yet.
| /** | ||
| * @brief The number of dimensions in the data block. | ||
| */ | ||
| SizeType dimension; /**< The number of dimensions in the data block. */ |
There was a problem hiding this comment.
Why do we need this variable? Wouldn't the number of dimensions be given by the size variable?
There was a problem hiding this comment.
At least how it was currently set up, size is always an array of 3 values, and the dimension value is obtained by creating an H5 dataspace and retrieving its dimensions (see here). If the number of dimensions is 1, the size[1] and size[2] values are set to be 1. I'm don't think the size variable needs to necessarily work this way if we want to change the number of values it contains depending on the dimensionality of the input data.
|
@stephprince thanks for all the updates. The code looks overall very clean and is easy to read 👍 I've added few additional comments. Some of them are quick fixes and others are probably a bit more work. Please feel free to turn any items that you think are best addressed in separate PR's into new issues and we can work through them separately. Overall, I think this PR looks good. Once tests are passing and we have triaged the remaining items, this PR seems good to merge. |
Co-authored-by: Oliver Ruebel <[email protected]>
|
@oruebel I think I addressed or responded to all the comments most relevant for this PR. And then I made issues for all the rest of the comments to address in separate PRs. |
Thanks for creating the issues and addressing the comments. I created a couple of additional issues to not further hold up this PR and approved. This looks good to merge and we can address the open issues in separate PRs. |
These changes will allow saving of electrode related information (e.g.
Device,ElectrodeGroup,ElectrodeTable) to an NWB File. Some of the additional features added to support this:NWBDataTypes.cppfiles to supportContainer,DynamicTable,Data,VectorData,ElementIdentifiers,Device, andElectrodeGroupclasses. Not all methods related to the base classes have been implemented yet beyond what was required for the electrodes information saving.LinksandReferencesto HDF5 files