From 30bbbfda78d9a41905a93876575d96a4ea20662e Mon Sep 17 00:00:00 2001 From: ehennestad Date: Tue, 17 Sep 2024 22:57:58 +0200 Subject: [PATCH 1/2] Fix: bug in nwbToTable (Issue #597 ) --- +types/+util/+dynamictable/nwbToTable.m | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/+types/+util/+dynamictable/nwbToTable.m b/+types/+util/+dynamictable/nwbToTable.m index a450f59b..734f5372 100644 --- a/+types/+util/+dynamictable/nwbToTable.m +++ b/+types/+util/+dynamictable/nwbToTable.m @@ -2,7 +2,7 @@ %NWBTOTABLE converts from a NWB DynamicTable to a MATLAB table % % MATLABTABLE = NWBTOTABLE(T) converts object T of class types.core.DynamicTable -% into a MATLAB Tale +% into a MATLAB Table % % MATLABTABLE = NWBTOTABLE(T, INDEX) If INDEX is FALSE, includes rows referenced by a % DynamicTableRegion as nested subtables @@ -50,10 +50,10 @@ ); % deal with DynamicTableRegion columns when index is false -columns = DynamicTable.colnames; -i = 1; -while i < length(columns) - cn = DynamicTable.colnames{i}; +[columns, remainingColumns] = deal(DynamicTable.colnames); + +for i = 1:length(columns) + cn = columns{i}; if isprop(DynamicTable, cn) cv = DynamicTable.(cn); elseif isprop(DynamicTable, 'vectorindex') && DynamicTable.vectorindex.isKey(cn) % Schema version < 2.3.0 @@ -71,15 +71,17 @@ cv{r,1} = ref_table.getRow(row_idxs(r)+1); end matlabTable.(cn) = cv; - columns(i) = []; + remainingColumns(i) = []; else - i = i+1; + % pass end end % append remaining columns to table % making the assumption that length of ids reflects table height matlabTable = [matlabTable DynamicTable.getRow( ... 1:length(ids), ... - 'columns', columns ... + 'columns', remainingColumns ... )]; +% Update the columns order to be the same as the original +matlabTable = matlabTable(:, [{'id'}, columns]); From 79e76a106971a6cd3c46df4c988c4b360aecd842 Mon Sep 17 00:00:00 2001 From: ehennestad Date: Sun, 22 Sep 2024 12:03:45 +0200 Subject: [PATCH 2/2] Add test, fix bug Add test that passes with this fix but would have filed before Fix bug in nwbToTable, should not have deleted elements of array using for loop indices --- +tests/+unit/dynamicTableTest.m | 72 +++++++++++++++++++++++++ +types/+util/+dynamictable/nwbToTable.m | 2 +- 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 +tests/+unit/dynamicTableTest.m diff --git a/+tests/+unit/dynamicTableTest.m b/+tests/+unit/dynamicTableTest.m new file mode 100644 index 00000000..e82267e2 --- /dev/null +++ b/+tests/+unit/dynamicTableTest.m @@ -0,0 +1,72 @@ +function tests = dynamicTableTest() + tests = functiontests(localfunctions); +end + +function setupOnce(testCase) + rootPath = fullfile(fileparts(mfilename('fullpath')), '..', '..'); + testCase.applyFixture(matlab.unittest.fixtures.PathFixture(rootPath)); + testCase.applyFixture(matlab.unittest.fixtures.WorkingFolderFixture); + generateCore('savedir', '.'); + rehash(); +end + +function setup(testCase) %#ok + % pass +end + +function testNwbToTableWithReferencedTablesAsRowIndices(testCase) + % The default mode for the toTable() method is to return the row indices + % for dynamic table regions. This test verifies that the data type of + % the converted table columns is int64, the default type for indices. + dtr_table = createDynamicTableWithTableRegionReferences(); + convertedTable = dtr_table.toTable(); + + testCase.verifyClass(convertedTable.dtr_col_a(1), 'int64') + testCase.verifyClass(convertedTable.dtr_col_b(1), 'int64') +end + +function testNwbToTableWithReferencedTablesAsTableRows(testCase) + % An alternative mode for the toTable() method is to return the referenced + % table rows for dynamic table regions as subtables. This test verifies that + % the data type of the converted table columns is table. + dtr_table = createDynamicTableWithTableRegionReferences(); + convertedTable = dtr_table.toTable(false); % Return + + row1colA = convertedTable.dtr_col_a(1); + row1colB = convertedTable.dtr_col_b(1); + if iscell(row1colA); row1colA = row1colA{1}; end + if iscell(row1colB); row1colB = row1colB{1}; end + + testCase.verifyClass(row1colA, 'table') + testCase.verifyClass(row1colB, 'table') +end + +% Non-test functions +function dtr_table = createDynamicTableWithTableRegionReferences() + % Create a dynamic table with two columns, where the data of each column is + % a dynamic table region referencing another dynamic table. + T = table([1;2;3], {'a';'b';'c'}, 'VariableNames', {'col1', 'col2'}); + T.Properties.VariableDescriptions = {'column #1', 'column #2'}; + + T = util.table2nwb(T); + + dtr_col_a = types.hdmf_common.DynamicTableRegion( ... + 'description', 'references multiple rows of earlier table', ... + 'data', [0; 1; 1; 0], ... # 0-indexed + 'table',types.untyped.ObjectView(T) ... % object view of target table + ); + + dtr_col_b = types.hdmf_common.DynamicTableRegion( ... + 'description', 'references multiple rows of earlier table', ... + 'data', [1; 2; 2; 1], ... # 0-indexed + 'table',types.untyped.ObjectView(T) ... % object view of target table + ); + + dtr_table = types.hdmf_common.DynamicTable( ... + 'description', 'test table with DynamicTableRegion', ... + 'colnames', {'dtr_col_a', 'dtr_col_b'}, ... + 'dtr_col_a', dtr_col_a, ... + 'dtr_col_b', dtr_col_b, ... + 'id',types.hdmf_common.ElementIdentifiers('data', [0; 1; 2; 3]) ... + ); +end diff --git a/+types/+util/+dynamictable/nwbToTable.m b/+types/+util/+dynamictable/nwbToTable.m index 734f5372..afe2f956 100644 --- a/+types/+util/+dynamictable/nwbToTable.m +++ b/+types/+util/+dynamictable/nwbToTable.m @@ -71,7 +71,7 @@ cv{r,1} = ref_table.getRow(row_idxs(r)+1); end matlabTable.(cn) = cv; - remainingColumns(i) = []; + remainingColumns = setdiff(remainingColumns, cn, 'stable'); else % pass end