NeurodataWithoutBorders / aqnwb

A C++ API for data acquisition using the NWB Format
https://neurodatawithoutborders.github.io/aqnwb/
Other
2 stars 0 forks source link

neurodata_type attribute not set correctly for subtypes of ``TimeSeries`` #108

Open oruebel opened 4 hours ago

oruebel commented 4 hours ago

The neurodata_type and namespace attributes are being set in TimeSeries.initialize

https://github.com/NeurodataWithoutBorders/aqnwb/blob/33e34810149b8bdecd7c5a26ed1cb2ae658d680b/src/nwb/base/TimeSeries.cpp#L35-L40

Subclasses, then first call the initialize method of their respective parent, e.g.,:

https://github.com/NeurodataWithoutBorders/aqnwb/blob/33e34810149b8bdecd7c5a26ed1cb2ae658d680b/src/nwb/ecephys/ElectricalSeries.cpp#L40-L42

https://github.com/NeurodataWithoutBorders/aqnwb/blob/33e34810149b8bdecd7c5a26ed1cb2ae658d680b/src/nwb/ecephys/SpikeEventSeries.cpp#L34-L36

As a result, for both ElectricalSeries and SpikeEventSeries the attribute neurodata_type in the file is actually being set to TimeSeries . The same issue applies for the namespace attribute, but we just don't see it as wrong because all the types are in the same namespace right now.

91 fixes this issue because it adds virtual getNamespace() and getTypename()`` functions that are being called to determine the type and because the functions are virtual they return the correct string even though the attribute in the file is still being created inTimeSeries.initialize` .

@stephprince it would be useful to discuss how we want to fix this on main. We could either: 1) try and push #91 and #85 over the finish line, or 2) try and implement and intermediate solution (ideally without creating a ton of conflicts with #91 and #85 )

oruebel commented 4 hours ago

Also, Container currently does not call createCommonNWBAttributes so it is missing those attributes when used directly. DynamicTable.initialize on-the-other-hand calls the function. We should probably move the call to createCommonNWBAttributes to the Container.initialize method.