Skip to content

Commit 19239e8

Browse files
ehennestadehennestad
authored
[BugFix] Improve DynamicTable getRow validation and height helpers (#812)
* Remove dynamic table mutation and early return on missing ids * Add better validation for "out of bounds" index in getRow * Make shared internal helpers for calculating column/table heights * Improve error message when requesting rows out of bounds * Update getRow.m Tighten input validation, row indices should be integers * Ensure getTableHeight returns a scalar height value
1 parent 3fe6873 commit 19239e8

9 files changed

Lines changed: 196 additions & 88 deletions

File tree

‎+tests/+unit/dynamicTableTest.m‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,39 @@ function testGetNegativeId(testCase)
4949
end
5050
end
5151

52+
function testGetRowRequiresIdOnlyWhenUseIdIsTrue(testCase)
53+
dynamicTable = types.hdmf_common.DynamicTable( ...
54+
'description', 'test table', ...
55+
'colnames', {'vec'}, ...
56+
'vec', types.hdmf_common.VectorData( ...
57+
'description', 'data column', ...
58+
'data', (1:3)') ...
59+
);
60+
61+
dynamicTable.id = [];
62+
63+
testCase.verifyWarningFree(@() dynamicTable.getRow(1));
64+
testCase.verifyError( ...
65+
@() dynamicTable.getRow(1, 'useId', true), ...
66+
'NWB:DynamicTable:GetRow:MissingId');
67+
end
68+
69+
function testGetRowThrowsSpecificErrorForOutOfBoundsIndex(testCase)
70+
dynamicTable = util.table2nwb(table([1; 2], ["a"; "b"]));
71+
72+
testCase.verifyError( ...
73+
@() dynamicTable.getRow(3), ...
74+
'NWB:DynamicTable:GetRow:RowOutOfBounds');
75+
end
76+
77+
function testGetRowThrowsSpecificErrorForEmptyTable(testCase)
78+
dynamicTable = types.hdmf_common.DynamicTable();
79+
80+
testCase.verifyError( ...
81+
@() dynamicTable.getRow(1), ...
82+
'NWB:DynamicTable:GetRow:RowOutOfBounds');
83+
end
84+
5285
function testNwbToTableWithReferencedTablesAsRowIndices(testCase)
5386
% The default mode for the toTable() method is to return the row indices
5487
% for dynamic table regions. This test verifies that the data type of

‎+types/+util/+dynamictable/+internal/getColumnHeight.m‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
function columnHeight = getColumnHeight(column)
2+
% getColumnHeight - Return the stored height of a DynamicTable column object.
3+
%
4+
% This helper inspects the underlying stored data shape only. It does not
5+
% resolve VectorIndex chains to determine DynamicTable row height.
6+
7+
if isempty(column)
8+
columnHeight = 0;
9+
else
10+
columnHeight = getDataHeight(column.data);
11+
end
12+
end
13+
14+
function columnHeight = getDataHeight(data)
15+
if isempty(data)
16+
columnHeight = 0;
17+
elseif isa(data, 'types.untyped.DataPipe')
18+
if data.isBound
19+
% Bound DataPipes can have an ambiguous inferred axis when a dataset
20+
% is extendable in multiple dimensions. Use the loaded MATLAB shape
21+
% instead, where DynamicTable row dimension maps to the last axis.
22+
dataDims = size(data);
23+
columnHeight = dataDims(end);
24+
elseif isempty(data.internal.data)
25+
columnHeight = 0;
26+
elseif ~isscalar(data.internal.data) && isvector(data.internal.data)
27+
columnHeight = length(data.internal.data); % datapipe axis can be misleading if vector.
28+
else
29+
columnHeight = size(data.internal.data, data.axis);
30+
end
31+
elseif isa(data, 'types.untyped.DataStub')
32+
columnHeight = data.dims(end);
33+
elseif isscalar(data) && isstruct(data) % compound type (struct)
34+
dataFieldNames = fieldnames(data);
35+
if isempty(dataFieldNames)
36+
columnHeight = 0;
37+
else
38+
columnHeight = zeros(size(dataFieldNames));
39+
for iField = 1:length(dataFieldNames)
40+
field = dataFieldNames{iField};
41+
columnHeight(iField) = getDataHeight(data.(field));
42+
end
43+
end
44+
elseif istable(data) % compound type (table)
45+
columnHeight = height(data);
46+
elseif isscalar(data) || ~isvector(data)
47+
columnHeight = size(data, ndims(data));
48+
else
49+
columnHeight = size(data, find(1 < size(data)));
50+
end
51+
end

‎+types/+util/+dynamictable/+internal/getColumnRowHeight.m‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
function [columnRowHeight, resolvedColumnName] = getColumnRowHeight(dynamicTable, columnName)
2+
% getColumnRowHeight - Return the row height for a DynamicTable column.
3+
%
4+
% For ragged columns, this follows VectorIndex links to the outermost index
5+
% column, whose height corresponds to the number of table rows.
6+
7+
arguments
8+
dynamicTable
9+
columnName {mustBeTextScalar}
10+
end
11+
12+
columnName = char(columnName);
13+
resolvedColumnName = types.util.dynamictable.internal.getOutermostIndexColumnName( ...
14+
dynamicTable, columnName);
15+
vector = getVector(dynamicTable, resolvedColumnName);
16+
columnRowHeight = types.util.dynamictable.internal.getColumnHeight(vector);
17+
end
18+
19+
function vector = getVector(dynamicTable, columnName)
20+
if isprop(dynamicTable, columnName)
21+
vector = dynamicTable.(columnName);
22+
elseif isprop(dynamicTable, 'vectorindex') && dynamicTable.vectorindex.isKey(columnName)
23+
vector = dynamicTable.vectorindex.get(columnName);
24+
elseif dynamicTable.vectordata.isKey(columnName)
25+
vector = dynamicTable.vectordata.get(columnName);
26+
else
27+
vector = [];
28+
end
29+
end

‎+types/+util/+dynamictable/+internal/getOutermostIndexColumnName.m‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
function outermostIndexColumnName = getOutermostIndexColumnName(dynamicTable, columnName)
2+
% getOutermostIndexColumnName - Resolve a column name to the outermost index column.
3+
%
4+
% Note: Relevant for ragged array columns
5+
6+
arguments
7+
dynamicTable
8+
columnName {mustBeTextScalar}
9+
end
10+
11+
columnName = char(columnName);
12+
13+
columnHistory = {};
14+
outermostIndexColumnName = columnName;
15+
while true
16+
indexName = types.util.dynamictable.getIndex(dynamicTable, outermostIndexColumnName);
17+
if isempty(indexName)
18+
return;
19+
end
20+
assert(~any(strcmp(columnHistory, indexName)), ...
21+
'NWB:DynamicTable:CheckConfig:InfiniteReferenceLoop', ...
22+
'Invalid Table shape detected: There is an infinite loop in your VectorIndex objects.');
23+
columnHistory{end+1} = indexName; %#ok<AGROW>
24+
outermostIndexColumnName = indexName;
25+
end
26+
end

‎+types/+util/+dynamictable/+internal/getTableHeight.m‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
function tableHeight = getTableHeight(dynamicTable)
2+
% getTableHeight - Return the inferred row height of a DynamicTable.
3+
%
4+
% This helper returns the effective table height. It differs from
5+
% `getColumnHeight`, which only inspects the stored height of a single
6+
% column object.
7+
8+
arguments
9+
dynamicTable
10+
end
11+
12+
if ~isempty(dynamicTable.id)
13+
tableHeight = types.util.dynamictable.internal.getColumnHeight(dynamicTable.id);
14+
return;
15+
end
16+
17+
if isempty(dynamicTable.colnames)
18+
tableHeight = 0;
19+
return;
20+
end
21+
22+
tableHeight = types.util.dynamictable.internal.getColumnRowHeight( ...
23+
dynamicTable, dynamicTable.colnames{1});
24+
tableHeight = unique(tableHeight);
25+
26+
assert(isscalar(tableHeight), ...
27+
'NWB:DynamicTable:GetRow:InvalidShape', ...
28+
['Cannot determine DynamicTable row height because one or more ', ...
29+
'compound column fields have inconsistent heights.']);
30+
end

‎+types/+util/+dynamictable/addVarargColumn.m‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ function addVarargColumn(DynamicTable, varargin)
2222

2323
% get current table height - assume id length reflects table height
2424
if ~isempty(DynamicTable.colnames)
25-
tableHeight = types.util.dynamictable.getColumnHeight(DynamicTable.id);
25+
tableHeight = types.util.dynamictable.internal.getColumnHeight(DynamicTable.id);
2626
end
2727

2828
% If adding the first column, initialize the id with 0-indexed values
@@ -36,7 +36,7 @@ function addVarargColumn(DynamicTable, varargin)
3636
firstColData = newVectorData.(indexName);
3737
end
3838

39-
newTableHeight = types.util.dynamictable.getColumnHeight(firstColData);
39+
newTableHeight = types.util.dynamictable.internal.getColumnHeight(firstColData);
4040
types.util.dynamictable.internal.initDynamicTableId(DynamicTable, newTableHeight);
4141
tableHeight = newTableHeight;
4242
end
@@ -53,7 +53,7 @@ function addVarargColumn(DynamicTable, varargin)
5353
else
5454
heightColumn = newVectorData.(indexName);
5555
end
56-
currentColumnHeight = types.util.dynamictable.getColumnHeight(heightColumn);
56+
currentColumnHeight = types.util.dynamictable.internal.getColumnHeight(heightColumn);
5757

5858
validateColumnHeight(new_cn, currentColumnHeight, tableHeight)
5959
end

‎+types/+util/+dynamictable/checkConfig.m‎

Lines changed: 4 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,9 @@ function checkConfig(DynamicTable, ignoreList)
3838
columnHeights = zeros(length(columns), 1);
3939
columnNames = strings(length(columns), 1);
4040
for iCol = 1:length(columns)
41-
columnName = retrieveHighestIndex(DynamicTable, columns{iCol});
42-
columnHeight = unique(types.util.dynamictable.getColumnHeight(getVector(DynamicTable, columnName)));
41+
[columnHeight, columnName] = types.util.dynamictable.internal.getColumnRowHeight( ...
42+
DynamicTable, columns{iCol});
43+
columnHeight = unique(columnHeight);
4344

4445
assert(isscalar(columnHeight), ...
4546
'NWB:DynamicTable:CheckConfig:InvalidShape', ...
@@ -65,7 +66,7 @@ function checkConfig(DynamicTable, ignoreList)
6566
return;
6667
end
6768

68-
numIds = types.util.dynamictable.getColumnHeight(DynamicTable.id);
69+
numIds = types.util.dynamictable.internal.getColumnHeight(DynamicTable.id);
6970
assert(tableHeight == numIds, ...
7071
'NWB:DynamicTable:CheckConfig:InvalidId', ...
7172
'Special column `id` of DynamicTable needs to match the detected height of %d. Found %d IDs.', ...
@@ -107,34 +108,6 @@ function checkConfig(DynamicTable, ignoreList)
107108

108109
end
109110

110-
function Vector = getVector(DynamicTable, column)
111-
if isprop(DynamicTable, column)
112-
Vector = DynamicTable.(column);
113-
elseif isprop(DynamicTable, 'vectorindex') && isKey(DynamicTable.vectorindex, column)
114-
Vector = DynamicTable.vectorindex.get(column);
115-
elseif isKey(DynamicTable.vectordata, column)
116-
Vector = DynamicTable.vectordata.get(column);
117-
else
118-
Vector = [];
119-
end
120-
end
121-
122-
function highestName = retrieveHighestIndex(DynamicTable, column)
123-
columnHistory = {};
124-
highestName = column;
125-
while true
126-
indexName = types.util.dynamictable.getIndex(DynamicTable, highestName);
127-
if isempty(indexName)
128-
return;
129-
end
130-
assert(~any(strcmp(columnHistory, indexName)), ...
131-
'NWB:DynamicTable:CheckConfig:InfiniteReferenceLoop', ...
132-
'Invalid Table shape detected: There is an infinite loop in your VectorIndex objects.');
133-
columnHistory{end+1} = indexName;
134-
highestName = indexName;
135-
end
136-
end
137-
138111
function colnames = cleanColumnNames(colnames)
139112
%CLEANCOLUMNNAMES removes the null character from column names.
140113
assert(iscellstr(colnames) || ischar(colnames), ...

‎+types/+util/+dynamictable/getColumnHeight.m‎

Lines changed: 0 additions & 46 deletions
This file was deleted.

‎+types/+util/+dynamictable/getRow.m‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010

1111
validateattributes(DynamicTable,...
1212
{'types.core.DynamicTable', 'types.hdmf_common.DynamicTable'}, {'scalar'});
13-
validateattributes(ind, {'numeric'}, {'vector'});
13+
validateattributes(ind, {'numeric'}, {'integer', 'vector'});
1414

1515
p = inputParser;
1616
addParameter(p, 'columns', DynamicTable.colnames, @(x)iscellstr(x));
@@ -20,15 +20,14 @@
2020
columns = p.Results.columns;
2121
row = cell(1, length(columns));
2222

23-
if isempty(DynamicTable.id)
24-
DynamicTable.id = types.hdmf_common.ElementIdentifiers();
25-
return;
26-
end
27-
2823
if p.Results.useId
24+
assert(~isempty(DynamicTable.id), ...
25+
'NWB:DynamicTable:GetRow:MissingId', ...
26+
'Cannot retrieve rows by `id` because the DynamicTable has no `id` column.');
2927
ind = getIndById(DynamicTable, ind);
3028
else
3129
validateattributes(ind, {'numeric'}, {'positive', 'vector'});
30+
validateRowIndices(DynamicTable, ind);
3231
end
3332

3433
for i = 1:length(columns)
@@ -103,7 +102,11 @@
103102
row{i} = rowStruct .';
104103
end
105104
end
106-
subTable = table(row{:}, 'VariableNames', columns);
105+
if isempty(columns)
106+
subTable = table('Size', [numel(ind), 0], 'VariableTypes', {}, 'VariableNames', {});
107+
else
108+
subTable = table(row{:}, 'VariableNames', columns);
109+
end
107110
end
108111

109112
function selected = select(DynamicTable, colIndStack, matInd)
@@ -219,8 +222,17 @@
219222
'Invalid ids found. If you wish to use row indices directly, remove the `useId` flag.');
220223
end
221224

225+
function validateRowIndices(dynamicTable, rowIndices)
226+
tableHeight = types.util.dynamictable.internal.getTableHeight(dynamicTable);
227+
228+
assert(all(rowIndices <= tableHeight), ...
229+
'NWB:DynamicTable:GetRow:RowOutOfBounds', ...
230+
'Requested row index (%s) exceeds the DynamicTable height of %d.', ...
231+
strjoin(compose('%d', rowIndices(rowIndices > tableHeight) ), ', '), tableHeight);
232+
end
233+
222234
function ME = InvalidVectorDataShapeError(column_name)
223235
ME = MException('NWB:DynamicTable:InvalidVectorDataShape', ...
224236
sprintf( ['Array data for column "%s" has a shape which do ', ...
225237
'not match the number of rows in the dynamic table.'], column_name ));
226-
end
238+
end

0 commit comments

Comments
 (0)