NeurodataWithoutBorders / matnwb

A Matlab interface for reading and writing NWB files
BSD 2-Clause "Simplified" License
49 stars 32 forks source link

[Bug]: checkConfig for dynamictable does not detect size of TimeSeriesReferenceVectorData object properly #432

Closed mfeyerab closed 2 years ago

mfeyerab commented 2 years ago

What happened?

Hello together,

since the new release I have trouble creating objects that are dynamic tables and also have a TimeSeriesReferenceVectorData object such as IntracellularStimuliTable and IntracellularResponsesTable. No matter how I create TimeSeriesReferenceVectorData object the number of rows do not match up with the IDs, because they are not detected correctly. TimeSeriesReferenceVectorData objects that were created with a structure for the data property always have the height 1 (as in the example I attach), whereas when I create them with a table the height is number of columns of that table (i.e. 3 for 'idx_start', 'count', 'timeseries'). It looks to me like that the revised checkConfig function is simply missing code to asses the size of the object correctly, or am I getting something completely wrong here? I do not have a strong coding background, so I would really appreciate if someone here could clarify this issue to me. The minimum example is based on code from the current intracellular tutorial.

Steps to Reproduce

ccss = types.core.VoltageClampStimulusSeries( ...
    'data', [1, 2, 3, 4, 5], ...
    'sweep_number', uint64(15)...
);

vcs = types.core.VoltageClampSeries( ...
    'data', [0.1, 0.2, 0.3, 0.4, 0.5], ...
    'sweep_number', uint64(15) ...
);

ic_rec_table.stimuli = types.core.IntracellularStimuliTable( ...
    'description', 'Table for storing intracellular stimulus related metadata.', ...
    'colnames', {'stimulus'}, ...
    'id', types.hdmf_common.ElementIdentifiers( ...
        'data', int64([0, 1, 2]) ...
    ), ...
    'stimulus', types.core.TimeSeriesReferenceVectorData( ...
        'description', 'Column storing the reference to the recorded stimulus for the recording (rows)', ...
        'data', struct( ...
            'idx_start', [0, 1, -1], ...
            'count', [5, 3, -1], ...
            'timeseries', [ ...
                types.untyped.ObjectView(ccss), ...
                types.untyped.ObjectView(ccss), ...
                types.untyped.ObjectView(vcs) ...
            ] ...
        )...
    )...
);

Error Message

Error using types.util.dynamictable.checkConfig (line 60)
Special column `id` of DynamicTable needs to match the detected height
of 1. Found 3 IDs.
Error in types.core.IntracellularStimuliTable (line 28)
            types.util.dynamictable.checkConfig(obj);
Error in minimum (line 11)
ic_rec_table.stimuli = types.core.IntracellularStimuliTable( ...

Operating System

Windows

Matlab Version

'9.9.0.1495850 (R2020b) Update 1'

Code of Conduct

lawrence-mbf commented 2 years ago

It looks like the checkConfig doesn't check that form of compound type (struct with arrays).

lawrence-mbf commented 2 years ago

@mfeyerab If you're interested in confirming: this branch: https://github.com/NeurodataWithoutBorders/matnwb/tree/432-checkconfig-compound-height appears to resolve your issue.

mfeyerab commented 2 years ago

@lawrence-mbf thank you for resolving this particular issue and sorry for me late response time. I fear though the problem goes deeper. I still run at least into one more issue when creating NWB files with/for intracellular data. I am about to create another bug report. I am not really a developer, so I hope that is the best way to go about it. I am a bit puzzled though that the new changes around dynamic tables in the latest release are in conflict with several MATNWB objects used for Intracellular data.

lawrence-mbf commented 2 years ago

Hi @mfeyerab no problem, go ahead and mention that in your ticket. It's always good to get more coverage on types our developers might not use often or didn't consider for testing.