Skip to content

Commit faf3801

Browse files
ehennestadCopilotbendichter
authored
[codex] Improve dynamic table validation and colnames sync (#818)
* First pass by codex * Update dynamicTableTest.m Fix broken test * Fix checkConfig to apply ignoreList before missingColumnNames assertion Agent-Logs-Url: https://github.com/NeurodataWithoutBorders/matnwb/sessions/f5f16c03-9489-4eee-8c7f-127005b690ce Co-authored-by: ehennestad <17237719+ehennestad@users.noreply.github.com> * validateColnames should accept string values * Update docstring in checkConfig to reflect recent changes * Simplify type normalization of colnames in validateColnames --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ehennestad <17237719+ehennestad@users.noreply.github.com> Co-authored-by: Ben Dichter <ben.dichter@gmail.com>
1 parent f6a4dbc commit faf3801

25 files changed

Lines changed: 449 additions & 77 deletions

‎+file/fillClass.m‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,11 @@
164164
superClassProps, ...
165165
class, ...
166166
inherited);
167-
setterFcns = file.fillSetters(setdiff(nonInherited, union(readonly, hiddenAndReadonly)), classprops);
167+
setterFcns = file.fillSetters( ...
168+
setdiff(nonInherited, union(readonly, hiddenAndReadonly)), ...
169+
classprops, ...
170+
name, ...
171+
namespace);
168172
validatorFcns = file.fillValidators(allProperties, classprops, namespace, namespace.getFullClassName(name), inherited);
169173
exporterFcns = file.fillExport(nonInherited, class, superclassNames{1}, required, classprops);
170174
methodBody = strjoin({constructorBody...

‎+file/fillConstructor.m‎

Lines changed: 2 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
end
2929

3030
% Add custom validation for DynamicTable and its descendant classes
31-
if isDynamicTableDescendant(name, namespace)
31+
if file.isDynamicTableDescendant(name, namespace)
3232
constructorElements{end+1} = ' types.util.dynamictable.checkConfig(obj);';
3333
end
3434

@@ -270,26 +270,6 @@
270270
docString = char( strjoin(docString, newline) );
271271
end
272272

273-
function tf = isDynamicTableDescendant(name, namespace)
274-
% Check if name is DynamicTable or if name is for a type that inherits from DynamicTable
275-
276-
tf = false;
277-
278-
if strcmp(name, 'DynamicTable')
279-
tf = true;
280-
return
281-
end
282-
283-
ancestry = namespace.getRootBranch(name);
284-
for iAncestor = 1:length(ancestry)
285-
ParentRaw = ancestry{iAncestor};
286-
% this is always true, we just use the proper index as typedefs may vary.
287-
typeDefInd = isKey(ParentRaw, namespace.TYPEDEF_KEYS);
288-
ancestorName = ParentRaw(namespace.TYPEDEF_KEYS{typeDefInd});
289-
tf = tf || strcmp(ancestorName, 'DynamicTable');
290-
end
291-
end
292-
293273
% Todo: Mostly duplicate code from file.fillProps. Should consolidate
294274
function typeStr = getTypeStr(prop)
295275
if ischar(prop)
@@ -344,4 +324,4 @@ function assertValidRefType(referenceType)
344324
word (1,:) char
345325
end
346326
word(1) = upper(word(1));
347-
end
327+
end

‎+file/fillCustomConstraint.m‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,13 @@
3434
' end\n', ...
3535
'end'] );
3636

37+
case "DynamicTable"
38+
customConstraintStr = sprintf( [...
39+
'function checkCustomConstraint(obj)\n', ...
40+
' checkCustomConstraint@types.untyped.MetaClass(obj)\n', ...
41+
' types.util.dynamictable.checkConfig(obj)\n', ...
42+
'end'] );
43+
3744
otherwise
3845
customConstraintStr = '';
3946
end

‎+file/fillSetters.m‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
1-
function fsstr = fillSetters(propnames, classprops)
1+
function fsstr = fillSetters(propnames, classprops, typeName, namespace)
22
fsstr = cell(size(propnames));
33
for i=1:length(propnames)
44
nm = propnames{i};
55
prop = classprops(nm);
6-
postsetFunctionStr = resolvePostsetFunction(nm, prop);
6+
postsetFunctionStr = resolvePostsetFunction(nm, prop, typeName, namespace);
77
if isempty(postsetFunctionStr)
88
fsstr{i} = strjoin({...
99
['function set.' nm '(obj, val)']...
@@ -22,9 +22,10 @@
2222
fsstr = strjoin(fsstr, newline);
2323
end
2424

25-
function postsetFunctionStr = resolvePostsetFunction(propname, prop)
25+
function postsetFunctionStr = resolvePostsetFunction(propName, prop, typeName, namespace)
2626

27-
postsetFunctionStr = '';
27+
hookInfo = file.getPropertyHooks(propName, prop, typeName, namespace);
28+
postsetStatements = hookInfo.PostsetStatements;
2829

2930
if isa(prop, 'file.Attribute')
3031

@@ -38,11 +39,11 @@
3839

3940
conditionStr = sprintf(...
4041
'if isempty(obj.%s) && ~isempty(obj.%s)', ...
41-
parentname, propname);
42+
parentname, propName);
4243

4344
warnIfDependencyMissingString = sprintf(...
4445
'obj.warnIfAttributeDependencyMissing(''%s'', ''%s'')', ...
45-
propname, parentname);
46+
propName, parentname);
4647

4748
syncPromotedDatasetAttributeString = '';
4849
if prop.promoted_to_container
@@ -53,19 +54,27 @@
5354
' elseif ~isempty(obj.%1$s.%2$s)\n' ...
5455
' obj.%3$s = obj.%1$s.%2$s;\n' ...
5556
' end\n' ...
56-
'end'], parentname, prop.name, propname);
57+
'end'], parentname, prop.name, propName);
5758
end
58-
59-
postsetLines = {...
60-
sprintf('function postset_%s(obj)', propname), ...
61-
file.addSpaces(conditionStr, 4), ...
62-
file.addSpaces(warnIfDependencyMissingString, 8), ...
63-
file.addSpaces('end', 4), ...
64-
'end'};
59+
60+
postsetStatements = [postsetStatements, ...
61+
{conditionStr}, ...
62+
{file.addSpaces(warnIfDependencyMissingString, 4)}, ...
63+
{'end'}];
6564
if ~isempty(syncPromotedDatasetAttributeString)
66-
postsetLines = [postsetLines(1:end-1), {file.addSpaces(syncPromotedDatasetAttributeString, 4)}, postsetLines(end)];
65+
postsetStatements{end+1} = syncPromotedDatasetAttributeString;
6766
end
68-
postsetFunctionStr = strjoin(postsetLines, newline);
6967
end
7068
end
69+
70+
if isempty(postsetStatements)
71+
postsetFunctionStr = '';
72+
return
73+
end
74+
75+
postsetBody = strjoin(postsetStatements, newline);
76+
postsetLines = {sprintf('function postset_%s(obj)', propName), ...
77+
file.addSpaces(postsetBody, 4), ...
78+
'end'};
79+
postsetFunctionStr = strjoin(postsetLines, newline);
7180
end

‎+file/fillValidators.m‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,9 @@
2121
validationBody = fillDtypeValidation(nm, prop, namespaceReg);
2222
end
2323

24+
hookInfo = file.getPropertyHooks(nm, prop, classNameParts(className), namespaceReg);
25+
validationBody = appendLines(validationBody, hookInfo.ValidatorLines);
26+
2427
headerStr = ['function val = validate_' nm '(obj, val)'];
2528
if isempty(validationBody)
2629
functionStr = [headerStr newline 'end'];
@@ -32,6 +35,25 @@
3235
end
3336
end
3437

38+
function typeName = classNameParts(className)
39+
classNameSplit = strsplit(className, '.');
40+
typeName = classNameSplit{end};
41+
end
42+
43+
function combinedLines = appendLines(validationBody, additionalLines)
44+
if isempty(additionalLines)
45+
combinedLines = validationBody;
46+
return
47+
end
48+
49+
validationLines = {};
50+
if ~isempty(strtrim(validationBody))
51+
validationLines = {strtrim(validationBody)};
52+
end
53+
54+
combinedLines = strjoin([validationLines, additionalLines], newline);
55+
end
56+
3557
function unitValidationStr = fillUnitValidation(name, prop, namespaceReg)
3658
unitValidationStr = '';
3759
if ~isscalar(prop) || (isa(prop, 'file.Link') && prop.isConstrainedSet)

‎+file/getPropertyHooks.m‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
function hooks = getPropertyHooks(propName, prop, typeName, namespace)
2+
%GETPROPERTYHOOKS Return custom generator hooks for property validators/setters.
3+
4+
hooks = struct( ...
5+
'ValidatorLines', {{}}, ...
6+
'PostsetStatements', {{}});
7+
8+
fullClassName = namespace.getFullClassName(typeName);
9+
10+
if strcmp(fullClassName, 'types.hdmf_common.DynamicTable') ...
11+
&& strcmp(propName, 'colnames')
12+
hooks.ValidatorLines = { ...
13+
'val = types.util.dynamictable.validateColnames(val);' };
14+
end
15+
16+
if file.isDynamicTableDescendant(typeName, namespace) ...
17+
&& isSchemaDefinedDynamicTableColumn(propName, prop)
18+
hooks.PostsetStatements = { ...
19+
sprintf('types.util.dynamictable.syncNamedColumn(obj, ''%s'');', propName) };
20+
end
21+
end
22+
23+
function tf = isSchemaDefinedDynamicTableColumn(propName, prop)
24+
tf = isa(prop, 'file.Dataset') ...
25+
&& ~prop.isConstrainedSet ...
26+
&& ~strcmp(propName, 'id') ...
27+
&& ~endsWith(propName, '_index');
28+
end

‎+file/isDynamicTableDescendant.m‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
function tf = isDynamicTableDescendant(name, namespace)
2+
%ISDYNAMICTABLEDESCENDANT Check if a type inherits from DynamicTable.
3+
4+
tf = false;
5+
6+
if strcmp(name, 'DynamicTable')
7+
tf = true;
8+
return
9+
end
10+
11+
ancestry = namespace.getRootBranch(name);
12+
for iAncestor = 1:length(ancestry)
13+
parentRaw = ancestry{iAncestor};
14+
typeDefIndex = isKey(parentRaw, namespace.TYPEDEF_KEYS);
15+
ancestorName = parentRaw(namespace.TYPEDEF_KEYS{typeDefIndex});
16+
tf = tf || strcmp(ancestorName, 'DynamicTable');
17+
end
18+
end

‎+tests/+unit/dynamicTableTest.m‎

Lines changed: 67 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,21 @@ function testEmptyTable(testCase)
1616
'description', 'test dynamic table column',...
1717
'id', types.hdmf_common.ElementIdentifiers('data', (0:2)'));
1818

19-
% validate that properties checking works (start_time is a
20-
% property of TimeIntervals)
21-
testCase.verifyError(@()types.core.TimeIntervals(...
22-
'description', 'test error', ...
23-
'start_time', types.hdmf_common.VectorData( ...
19+
% Verify that start_time is added to colnames when creating an
20+
% empty VectorData column for the start_time property
21+
try
22+
timeIntervals = types.core.TimeIntervals(...
23+
'description', 'test error', ...
24+
'start_time', types.hdmf_common.VectorData( ...
2425
'description', 'start time column', ...
25-
'data', (1:5)')), ...
26-
'NWB:DynamicTable:CheckConfig:ColumnNamesMismatch');
26+
'data', (1:5)' ...
27+
));
28+
testCase.verifyTrue(contains(timeIntervals.colnames, 'start_time'), ...
29+
'Expected colnames to include start_time')
30+
catch
31+
testCase.verifyFail(['Failed to create an empty TimeIntervals ' ...
32+
'table with start_time column.'])
33+
end
2734

2835
% validate that "vectordata" set checking works.
2936
testCase.verifyError(@()types.core.TimeIntervals(...
@@ -350,6 +357,59 @@ function testAddDataPipeWithWrongHeightToDynamicTable(testCase)
350357
@() dynamicTable.addColumn('columnB', columnB), ...
351358
'NWB:DynamicTable:AddColumn:MissingRows');
352359
end
360+
361+
function testDuplicateColnamesFailValidation(testCase)
362+
testCase.verifyError( ...
363+
@() types.hdmf_common.DynamicTable( ...
364+
'description', 'test table with duplicate colnames', ...
365+
'colnames', {'columnA', 'columnA'}), ...
366+
'NWB:DynamicTable:DuplicateColumnNames');
367+
end
368+
369+
function testDirectAssignmentSyncsNamedColumnsToColnames(testCase)
370+
dynamicTable = types.core.TimeIntervals( ...
371+
'description', 'test direct assignment');
372+
373+
dynamicTable.start_time = types.hdmf_common.VectorData( ...
374+
'description', 'start time column', ...
375+
'data', single((1:3)'));
376+
dynamicTable.stop_time = types.hdmf_common.VectorData( ...
377+
'description', 'stop time column', ...
378+
'data', single((2:4)'));
379+
380+
testCase.verifyEqual(dynamicTable.colnames, {'start_time', 'stop_time'});
381+
end
382+
383+
function testCheckConfigDetectsColumnsMissingFromColnames(testCase)
384+
dynamicTable = types.hdmf_common.DynamicTable( ...
385+
'description', 'test table with unregistered column', ...
386+
'colnames', {'columnA'});
387+
388+
dynamicTable.vectordata.set('columnB', types.hdmf_common.VectorData( ...
389+
'description', 'column b', ...
390+
'data', (1:3)'));
391+
392+
testCase.verifyError( ...
393+
@() types.util.dynamictable.checkConfig(dynamicTable), ...
394+
'NWB:DynamicTable:CheckConfig:ColumnNamesMismatch');
395+
end
396+
397+
function testExportDetectsColumnsMissingFromColnames(testCase)
398+
fileName = testCase.getRandomFilename();
399+
nwb = tests.factory.NWBFile();
400+
401+
dynamicTable = types.hdmf_common.DynamicTable( ...
402+
'description', 'test table with unregistered column', ...
403+
'colnames', {'columnA'});
404+
dynamicTable.vectordata.set('columnB', types.hdmf_common.VectorData( ...
405+
'description', 'column b', ...
406+
'data', (1:3)'));
407+
408+
nwb.acquisition.set('DynamicTable', dynamicTable);
409+
410+
testCase.verifyError(@() nwbExport(nwb, fileName), ...
411+
'NWB:CustomConstraintUnfulfilled');
412+
end
353413
end
354414

355415
methods (Static, Access=private)

0 commit comments

Comments
 (0)