Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

neurodata_type attribute not set correctly for subtypes of TimeSeries #108

Open
oruebel opened this issue Sep 20, 2024 · 2 comments
Open

Comments

@oruebel
Copy link
Contributor

oruebel commented Sep 20, 2024

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

void TimeSeries::initialize()
{
Container::initialize();
// setup attributes
m_io->createCommonNWBAttributes(m_path, "core", neurodataType, description);

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

void ElectricalSeries::initialize()
{
TimeSeries::initialize();

void SpikeEventSeries::initialize()
{
ElectricalSeries::initialize();

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 in TimeSeries.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
Copy link
Contributor Author

oruebel commented Sep 20, 2024

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.

@oruebel
Copy link
Contributor Author

oruebel commented Sep 21, 2024

It looks like this issue is also potentially "hiding" some validation errors because the objects are being validated as TimeSeries instead of as an ElectricalSeries so that extra fields for ElectricalSeries may not be checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant