Skip to content

Commit 95b5e5e

Browse files
authored
588 Add check on file export to ensure all required properties are filled out (#600)
* Update metaclass to instrospect required properties * Minor fixes to MetaClass wrt checking required properties * Display classname in warning for missing required properties * Add custom constraint checks (Issue #588 ) * Update multipleConstrainedTest, add required dataset in type * Update linkTest to pass required property check * Update anonTest to pass required property check * Add explanations for custom constraints * Add unittest to test changes made in this PR * Hardcode exceptions for warnIfMissingRequiredProperties + Fix syntax error (missing end) in nwbExportTest * Fix test for special case dataset starting_time in TimeSeries * Change test workflow to upload test results also if tests fails * Fix more issues with test workflow
1 parent 5cd6af6 commit 95b5e5e

12 files changed

+212
-31
lines changed

+file/fillClass.m

+5
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,11 @@
129129
'%% VALIDATORS' validatorFcns...
130130
'%% EXPORT' exporterFcns}, newline);
131131

132+
customConstraintStr = file.fillCustomConstraint(name);
133+
if ~isempty(customConstraintStr)
134+
methodBody = strjoin({methodBody, '%% CUSTOM CONSTRAINTS', customConstraintStr}, newline);
135+
end
136+
132137
if strcmp(name, 'DynamicTable')
133138
methodBody = strjoin({methodBody, '%% TABLE METHODS', file.fillDynamicTableMethods()}, newline);
134139
end

+file/fillCustomConstraint.m

+38
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
function customConstraintStr = fillCustomConstraint(nwbType)
2+
% fillCustomConstraint - Create functions to check custom constraints
3+
% These are constraints that can not be inferred from the nwb-schema
4+
%
5+
% Reference: https://github.com/NeurodataWithoutBorders/matnwb/issues/588
6+
7+
switch nwbType
8+
9+
case "TimeSeries"
10+
% Add method to validate constraint that either timestamps or
11+
% starting_time must be present
12+
customConstraintStr = sprintf( [...
13+
'function checkCustomConstraint(obj)\n', ...
14+
' assert(~isempty(obj.timestamps) || ~isempty(obj.starting_time), ...\n', ...
15+
' "''timestamps'' or ''starting_time'' must be specified")\n', ...
16+
' if ~isempty(obj.starting_time)\n', ...
17+
' assert(~isempty(obj.starting_time_rate), ...\n', ...
18+
' "''starting_time_rate'' must be specified when ''starting_time'' is specified")\n', ...
19+
' end\n', ...
20+
'end'] );
21+
22+
23+
case "ImageSeries"
24+
% If external_file is set, it does not make sense to fill out the
25+
% data property. However, data is a required property, so this
26+
% method will add a nan-array to the data property so that it passes
27+
% the requirement check on file export.
28+
customConstraintStr = sprintf( [...
29+
'function checkCustomConstraint(obj)\n', ...
30+
' if ~isempty(obj.external_file) && isempty(obj.data), ...\n', ...
31+
' obj.data = nan(1,1,2);\n', ...
32+
' end\n', ...
33+
'end'] );
34+
35+
otherwise
36+
customConstraintStr = '';
37+
end
38+
end

+tests/+unit/anonTest.m

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ function setup(testCase)
1717
end
1818

1919
function testAnonDataset(testCase)
20-
ag = types.anon.AnonGroup('ad', types.anon.AnonData());
20+
ag = types.anon.AnonGroup('ad', types.anon.AnonData('data', 0));
2121
nwbExpected = NwbFile(...
2222
'identifier', 'ANON',...
2323
'session_description', 'anonymous class schema testing',...

+tests/+unit/linkTest.m

+2
Original file line numberDiff line numberDiff line change
@@ -65,9 +65,11 @@ function testExternalResolution(testCase)
6565
expectedData = rand(100,1);
6666
stubDtr = types.hdmf_common.DynamicTableRegion(...
6767
'table', types.untyped.ObjectView('/acquisition/es1'),...
68+
'data', 1, ...
6869
'description', 'dtr stub that points to electrical series illegally'); % do not do this at home.
6970
expected = types.core.ElectricalSeries('data', expectedData,...
7071
'data_unit', 'volts', ...
72+
'timestamps', (1:100)', ...
7173
'electrodes', stubDtr);
7274
nwb.acquisition.set('es1', expected);
7375
nwb.export('test1.nwb');

+tests/+unit/multipleConstrainedTest.m

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ function testRoundabout(testCase)
2020
MultiSet = types.mcs.MultiSetContainer();
2121
MultiSet.something.set('A', types.mcs.ArbitraryTypeA());
2222
MultiSet.something.set('B', types.mcs.ArbitraryTypeB());
23-
MultiSet.something.set('Data', types.mcs.DatasetType());
23+
MultiSet.something.set('Data', types.mcs.DatasetType('data', ones(3,3)));
2424
nwbExpected = NwbFile(...
2525
'identifier', 'MCS', ...
2626
'session_description', 'multiple constrained schema testing', ...

+tests/+unit/nwbExportTest.m

+23-9
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,26 @@ function setupMethod(testCase)
3131
end
3232

3333
methods (Test)
34+
function testExportNwbFileWithMissingRequiredProperties(testCase)
35+
nwb = NwbFile();
36+
nwbFilePath = fullfile(testCase.OutputFolder, 'testfile.nwb');
37+
testCase.verifyError(@(file, path) nwbExport(nwb, nwbFilePath), ...
38+
'NWB:RequiredPropertyMissing')
39+
end
40+
41+
function testExportTimeseriesWithMissingTimestampsAndStartingTime(testCase)
42+
time_series = types.core.TimeSeries( ...
43+
'data', linspace(0, 0.4, 50), ...
44+
'description', 'a test series', ...
45+
'data_unit', 'n/a' ...
46+
);
47+
48+
testCase.NwbObject.acquisition.set('time_series', time_series);
49+
nwbFilePath = fullfile(testCase.OutputFolder, 'testfile.nwb');
50+
testCase.verifyError(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), ...
51+
'NWB:CustomConstraintUnfulfilled')
52+
end
53+
3454
function testExportDependentAttributeWithMissingParentA(testCase)
3555
testCase.NwbObject.general_source_script_file_name = 'my_test_script.m';
3656
nwbFilePath = fullfile(testCase.OutputFolder, 'test_part1.nwb');
@@ -42,22 +62,16 @@ function testExportDependentAttributeWithMissingParentA(testCase)
4262
testCase.verifyWarningFree(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath))
4363
end
4464

45-
function testExportDependentAttributeWithMissingParentB(testCase)
65+
function testExportTimeseriesWithoutStartingTimeRate(testCase)
4666
time_series = types.core.TimeSeries( ...
4767
'data', linspace(0, 0.4, 50), ...
48-
'starting_time_rate', 10.0, ...
68+
'starting_time', 1, ...
4969
'description', 'a test series', ...
5070
'data_unit', 'n/a' ...
5171
);
52-
5372
testCase.NwbObject.acquisition.set('time_series', time_series);
5473
nwbFilePath = fullfile(testCase.OutputFolder, 'test_part1.nwb');
55-
testCase.verifyWarning(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), 'NWB:DependentAttributeNotExported')
56-
57-
% Add value for dataset which attribute depends on and export again
58-
time_series.starting_time = 1;
59-
nwbFilePath = fullfile(testCase.OutputFolder, 'test_part2.nwb');
60-
testCase.verifyWarningFree(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath))
74+
testCase.verifyError(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), 'NWB:CustomConstraintUnfulfilled')
6175
end
6276
end
6377

+types/+core/ImageSeries.m

+6
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,12 @@
186186
end
187187
end
188188
end
189+
%% CUSTOM CONSTRAINTS
190+
function checkCustomConstraint(obj)
191+
if ~isempty(obj.external_file) && isempty(obj.data), ...
192+
obj.data = nan(1,1,2);
193+
end
194+
end
189195
end
190196

191197
end

+types/+core/TimeSeries.m

+9
Original file line numberDiff line numberDiff line change
@@ -415,6 +415,15 @@
415415
io.writeAttribute(fid, [fullpath '/timestamps/unit'], obj.timestamps_unit);
416416
end
417417
end
418+
%% CUSTOM CONSTRAINTS
419+
function checkCustomConstraint(obj)
420+
assert(~isempty(obj.timestamps) || ~isempty(obj.starting_time), ...
421+
"'timestamps' or 'starting_time' must be specified")
422+
if ~isempty(obj.starting_time)
423+
assert(~isempty(obj.starting_time_rate), ...
424+
"'starting_time_rate' must be specified when 'starting_time' is specified")
425+
end
426+
end
418427
end
419428

420429
end

+types/+untyped/MetaClass.m

+108-3
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,12 @@
1-
classdef MetaClass < handle
1+
classdef MetaClass < handle & matlab.mixin.CustomDisplay
22
properties (Hidden, SetAccess = private)
33
metaClass_fullPath;
44
end
5-
5+
6+
properties (Constant, Transient, Access = protected)
7+
REQUIRED containers.Map = containers.Map
8+
end
9+
610
methods
711
function obj = MetaClass(varargin)
812
end
@@ -43,6 +47,11 @@
4347

4448
methods
4549
function refs = export(obj, fid, fullpath, refs)
50+
% throwErrorIfCustomConstraintUnfulfilled is intentionally placed
51+
% before throwErrorIfMissingRequiredProps.
52+
% See file.fillCustomConstraint
53+
obj.throwErrorIfCustomConstraintUnfulfilled(fullpath)
54+
obj.throwErrorIfMissingRequiredProps(fullpath)
4655
obj.metaClass_fullPath = fullpath;
4756
%find reference properties
4857
propnames = properties(obj);
@@ -107,4 +116,100 @@ function warnIfPropertyAttributeNotExported(obj, propName, depPropName, fullpath
107116
warning(warningId, warningMessage) %#ok<SPWRN>
108117
end
109118
end
110-
end
119+
120+
methods (Access = protected) % Override matlab.mixin.CustomDisplay
121+
function str = getFooter(obj)
122+
obj.displayWarningIfMissingRequiredProps();
123+
str = '';
124+
end
125+
end
126+
127+
methods (Access = protected)
128+
function missingRequiredProps = checkRequiredProps(obj)
129+
missingRequiredProps = {};
130+
requiredProps = obj.getRequiredProperties();
131+
132+
for i = 1:numel(requiredProps)
133+
thisPropName = requiredProps{i};
134+
if isempty(obj.(thisPropName))
135+
missingRequiredProps{end+1} = thisPropName; %#ok<AGROW>
136+
end
137+
end
138+
end
139+
140+
function displayWarningIfMissingRequiredProps(obj)
141+
missingRequiredProps = obj.checkRequiredProps();
142+
143+
% Exception: 'file_create_date' is automatically added by the
144+
% matnwb API on export, so no need to warn if it is missing.
145+
if isa(obj, 'types.core.NWBFile')
146+
missingRequiredProps = setdiff(missingRequiredProps, 'file_create_date', 'stable');
147+
end
148+
149+
% Exception: 'id' of DynamicTable is automatically assigned if not
150+
% specified, so no need to warn if it is missing.
151+
if isa(obj, 'types.hdmf_common.DynamicTable')
152+
missingRequiredProps = setdiff(missingRequiredProps, 'id', 'stable');
153+
end
154+
155+
if ~isempty( missingRequiredProps )
156+
warnState = warning('backtrace', 'off');
157+
cleanerObj = onCleanup(@(s) warning(warnState));
158+
159+
propertyListStr = obj.prettyPrintPropertyList(missingRequiredProps);
160+
warning('NWB:RequiredPropertyMissing', ...
161+
'The following required properties are missing for instance for type "%s":\n%s', class(obj), propertyListStr)
162+
end
163+
end
164+
165+
function throwErrorIfMissingRequiredProps(obj, fullpath)
166+
missingRequiredProps = obj.checkRequiredProps();
167+
if ~isempty( missingRequiredProps )
168+
propertyListStr = obj.prettyPrintPropertyList(missingRequiredProps);
169+
error('NWB:RequiredPropertyMissing', ...
170+
'The following required properties are missing for instance for type "%s" at file location "%s":\n%s', class(obj), fullpath, propertyListStr)
171+
end
172+
end
173+
174+
function throwErrorIfCustomConstraintUnfulfilled(obj, fullpath)
175+
try
176+
obj.checkCustomConstraint()
177+
catch ME
178+
error('NWB:CustomConstraintUnfulfilled', ...
179+
'The following error was caught when exporting type "%s" at file location "%s":\n%s', class(obj), fullpath, ME.message)
180+
end
181+
end
182+
end
183+
184+
methods
185+
function checkCustomConstraint(obj) %#ok<MANU>
186+
% This method is meant to be overridden by subclasses that have
187+
% custom constraints that can not be inferred from the nwb schema.
188+
end
189+
end
190+
191+
methods (Access = private)
192+
function requiredProps = getRequiredProperties(obj)
193+
194+
% Introspectively retrieve required properties and add to
195+
% persistent map.
196+
197+
if isKey(obj.REQUIRED, class(obj) )
198+
requiredProps = obj.REQUIRED( class(obj) );
199+
else
200+
mc = metaclass(obj);
201+
propertyDescription = {mc.PropertyList.Description};
202+
isRequired = startsWith(propertyDescription, 'REQUIRED');
203+
requiredProps = {mc.PropertyList(isRequired).Name};
204+
obj.REQUIRED( class(obj) ) = requiredProps;
205+
end
206+
end
207+
end
208+
209+
methods (Static, Access = private)
210+
function propertyListStr = prettyPrintPropertyList(propertyNames)
211+
propertyListStr = compose(' %s', string(propertyNames));
212+
propertyListStr = strjoin(propertyListStr, newline);
213+
end
214+
end
215+
end

.github/workflows/run_codespell.yml

+1-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ on:
1212
branches:
1313
- master
1414
push:
15-
branches-ignore:
15+
branches:
1616
- master
1717

1818
jobs:

.github/workflows/run_tests.yml

+17-15
Original file line numberDiff line numberDiff line change
@@ -18,65 +18,67 @@ jobs:
1818
name: Run MATLAB tests
1919
runs-on: ubuntu-latest
2020
steps:
21-
- name: check out repository
21+
- name: Check out repository
2222
uses: actions/checkout@v4
23-
- name: install python
23+
- name: Install python
2424
uses: actions/setup-python@v5
2525
with:
2626
python-version: '3.10'
27-
- name: configure python env
27+
- name: Configure python env
2828
run: |
2929
python -m pip install -U pip
3030
pip install -r +tests/requirements.txt
3131
echo "HDF5_PLUGIN_PATH=$(python -c "import hdf5plugin; print(hdf5plugin.PLUGINS_PATH)")" >> "$GITHUB_ENV"
32-
- name: install MATLAB
32+
- name: Install MATLAB
3333
uses: matlab-actions/setup-matlab@v2
3434
with:
3535
release: R2024a # this is necessary to test dynamic filters
36-
- name: run tests
36+
- name: Run tests
3737
uses: matlab-actions/run-command@v2
3838
with:
3939
command: results = assertSuccess(nwbtest); assert(~isempty(results), 'No tests ran');
40-
- name: upload JUnit results
40+
- name: Upload JUnit results
41+
if: always()
4142
uses: actions/upload-artifact@v4
4243
with:
4344
name: test-results
4445
path: testResults.xml
4546
retention-days: 1
46-
- name: upload coverage results
47+
- name: Upload coverage results
48+
if: always()
4749
uses: actions/upload-artifact@v4
4850
with:
4951
name: test-coverage
5052
path: ./coverage.xml
5153
publish_junit:
52-
name: Publish JUnit Test Results
54+
name: Publish JUnit test results
5355
runs-on: ubuntu-latest
54-
if: ${{ always() }}
56+
if: always()
5557
needs: [run_tests]
5658
steps:
57-
- name: retrieve result files
59+
- name: Retrieve result files
5860
uses: actions/download-artifact@v4
5961
with:
6062
name: test-results
61-
- name: publish results
63+
- name: Publish test results
6264
uses: mikepenz/action-junit-report@v4
6365
with:
6466
report_paths: 'testResults.xml'
6567
publish_coverage:
66-
name: Publish Cobertura Test Coverage
68+
name: Publish Cobertura test coverage
6769
runs-on: ubuntu-latest
6870
needs: [run_tests]
6971
steps:
70-
- name: check out repository
72+
- name: Check out repository
7173
uses: actions/checkout@v4
7274
- name: retrieve code coverage files
7375
uses: actions/download-artifact@v4
7476
with:
7577
name: test-coverage
76-
- name: publish on Codecov
78+
- name: Publish on coverage results on Codecov
7779
uses: codecov/codecov-action@v4
7880
with:
7981
token: ${{ secrets.CODECOV_TOKEN }}
8082
files: coverage.xml
8183
name: codecov-matnwb
82-
verbose: true
84+
verbose: true

nwbtest.m

+1-1
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
coverageFile = fullfile(ws, 'coverage.xml');
6363
[installDir, ~, ~] = fileparts(mfilename('fullpath'));
6464

65-
ignoreFolders = {'tutorials', '+contrib', '+util', 'external_packages', '+tests'};
65+
ignoreFolders = {'tutorials', 'tools', '+contrib', '+util', 'external_packages', '+tests'};
6666
ignorePaths = {fullfile('+misc', 'generateDocs.m'), [mfilename '.m'], 'nwbClearGenerated.m'};
6767
mfilePaths = getMfilePaths(installDir, ignoreFolders, ignorePaths);
6868
if ~verLessThan('matlab', '9.3') && ~isempty(mfilePaths)

0 commit comments

Comments
 (0)