From 95b5e5e8d505f7a4e39e8100914c550a3957282c Mon Sep 17 00:00:00 2001 From: ehennestad Date: Sat, 2 Nov 2024 19:29:47 +0100 Subject: [PATCH] 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 --- +file/fillClass.m | 5 ++ +file/fillCustomConstraint.m | 38 +++++++++ +tests/+unit/anonTest.m | 2 +- +tests/+unit/linkTest.m | 2 + +tests/+unit/multipleConstrainedTest.m | 2 +- +tests/+unit/nwbExportTest.m | 32 +++++-- +types/+core/ImageSeries.m | 6 ++ +types/+core/TimeSeries.m | 9 ++ +types/+untyped/MetaClass.m | 111 ++++++++++++++++++++++++- .github/workflows/run_codespell.yml | 2 +- .github/workflows/run_tests.yml | 32 +++---- nwbtest.m | 2 +- 12 files changed, 212 insertions(+), 31 deletions(-) create mode 100644 +file/fillCustomConstraint.m diff --git a/+file/fillClass.m b/+file/fillClass.m index 8a8bec85..2f4c48eb 100644 --- a/+file/fillClass.m +++ b/+file/fillClass.m @@ -129,6 +129,11 @@ '%% VALIDATORS' validatorFcns... '%% EXPORT' exporterFcns}, newline); + customConstraintStr = file.fillCustomConstraint(name); + if ~isempty(customConstraintStr) + methodBody = strjoin({methodBody, '%% CUSTOM CONSTRAINTS', customConstraintStr}, newline); + end + if strcmp(name, 'DynamicTable') methodBody = strjoin({methodBody, '%% TABLE METHODS', file.fillDynamicTableMethods()}, newline); end diff --git a/+file/fillCustomConstraint.m b/+file/fillCustomConstraint.m new file mode 100644 index 00000000..5be7d9e8 --- /dev/null +++ b/+file/fillCustomConstraint.m @@ -0,0 +1,38 @@ +function customConstraintStr = fillCustomConstraint(nwbType) +% fillCustomConstraint - Create functions to check custom constraints +% These are constraints that can not be inferred from the nwb-schema +% +% Reference: https://github.com/NeurodataWithoutBorders/matnwb/issues/588 + + switch nwbType + + case "TimeSeries" + % Add method to validate constraint that either timestamps or + % starting_time must be present + customConstraintStr = sprintf( [... + 'function checkCustomConstraint(obj)\n', ... + ' assert(~isempty(obj.timestamps) || ~isempty(obj.starting_time), ...\n', ... + ' "''timestamps'' or ''starting_time'' must be specified")\n', ... + ' if ~isempty(obj.starting_time)\n', ... + ' assert(~isempty(obj.starting_time_rate), ...\n', ... + ' "''starting_time_rate'' must be specified when ''starting_time'' is specified")\n', ... + ' end\n', ... + 'end'] ); + + + case "ImageSeries" + % If external_file is set, it does not make sense to fill out the + % data property. However, data is a required property, so this + % method will add a nan-array to the data property so that it passes + % the requirement check on file export. + customConstraintStr = sprintf( [... + 'function checkCustomConstraint(obj)\n', ... + ' if ~isempty(obj.external_file) && isempty(obj.data), ...\n', ... + ' obj.data = nan(1,1,2);\n', ... + ' end\n', ... + 'end'] ); + + otherwise + customConstraintStr = ''; + end +end diff --git a/+tests/+unit/anonTest.m b/+tests/+unit/anonTest.m index 3bcad17d..c3ac5718 100644 --- a/+tests/+unit/anonTest.m +++ b/+tests/+unit/anonTest.m @@ -17,7 +17,7 @@ function setup(testCase) end function testAnonDataset(testCase) -ag = types.anon.AnonGroup('ad', types.anon.AnonData()); +ag = types.anon.AnonGroup('ad', types.anon.AnonData('data', 0)); nwbExpected = NwbFile(... 'identifier', 'ANON',... 'session_description', 'anonymous class schema testing',... diff --git a/+tests/+unit/linkTest.m b/+tests/+unit/linkTest.m index 1b74a569..4f5e688f 100644 --- a/+tests/+unit/linkTest.m +++ b/+tests/+unit/linkTest.m @@ -65,9 +65,11 @@ function testExternalResolution(testCase) expectedData = rand(100,1); stubDtr = types.hdmf_common.DynamicTableRegion(... 'table', types.untyped.ObjectView('/acquisition/es1'),... + 'data', 1, ... 'description', 'dtr stub that points to electrical series illegally'); % do not do this at home. expected = types.core.ElectricalSeries('data', expectedData,... 'data_unit', 'volts', ... + 'timestamps', (1:100)', ... 'electrodes', stubDtr); nwb.acquisition.set('es1', expected); nwb.export('test1.nwb'); diff --git a/+tests/+unit/multipleConstrainedTest.m b/+tests/+unit/multipleConstrainedTest.m index b9194445..bcbb5318 100644 --- a/+tests/+unit/multipleConstrainedTest.m +++ b/+tests/+unit/multipleConstrainedTest.m @@ -20,7 +20,7 @@ function testRoundabout(testCase) MultiSet = types.mcs.MultiSetContainer(); MultiSet.something.set('A', types.mcs.ArbitraryTypeA()); MultiSet.something.set('B', types.mcs.ArbitraryTypeB()); - MultiSet.something.set('Data', types.mcs.DatasetType()); + MultiSet.something.set('Data', types.mcs.DatasetType('data', ones(3,3))); nwbExpected = NwbFile(... 'identifier', 'MCS', ... 'session_description', 'multiple constrained schema testing', ... diff --git a/+tests/+unit/nwbExportTest.m b/+tests/+unit/nwbExportTest.m index abd45868..4f25e78e 100644 --- a/+tests/+unit/nwbExportTest.m +++ b/+tests/+unit/nwbExportTest.m @@ -31,6 +31,26 @@ function setupMethod(testCase) end methods (Test) + function testExportNwbFileWithMissingRequiredProperties(testCase) + nwb = NwbFile(); + nwbFilePath = fullfile(testCase.OutputFolder, 'testfile.nwb'); + testCase.verifyError(@(file, path) nwbExport(nwb, nwbFilePath), ... + 'NWB:RequiredPropertyMissing') + end + + function testExportTimeseriesWithMissingTimestampsAndStartingTime(testCase) + time_series = types.core.TimeSeries( ... + 'data', linspace(0, 0.4, 50), ... + 'description', 'a test series', ... + 'data_unit', 'n/a' ... + ); + + testCase.NwbObject.acquisition.set('time_series', time_series); + nwbFilePath = fullfile(testCase.OutputFolder, 'testfile.nwb'); + testCase.verifyError(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), ... + 'NWB:CustomConstraintUnfulfilled') + end + function testExportDependentAttributeWithMissingParentA(testCase) testCase.NwbObject.general_source_script_file_name = 'my_test_script.m'; nwbFilePath = fullfile(testCase.OutputFolder, 'test_part1.nwb'); @@ -42,22 +62,16 @@ function testExportDependentAttributeWithMissingParentA(testCase) testCase.verifyWarningFree(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath)) end - function testExportDependentAttributeWithMissingParentB(testCase) + function testExportTimeseriesWithoutStartingTimeRate(testCase) time_series = types.core.TimeSeries( ... 'data', linspace(0, 0.4, 50), ... - 'starting_time_rate', 10.0, ... + 'starting_time', 1, ... 'description', 'a test series', ... 'data_unit', 'n/a' ... ); - testCase.NwbObject.acquisition.set('time_series', time_series); nwbFilePath = fullfile(testCase.OutputFolder, 'test_part1.nwb'); - testCase.verifyWarning(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), 'NWB:DependentAttributeNotExported') - - % Add value for dataset which attribute depends on and export again - time_series.starting_time = 1; - nwbFilePath = fullfile(testCase.OutputFolder, 'test_part2.nwb'); - testCase.verifyWarningFree(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath)) + testCase.verifyError(@(f, fn) nwbExport(testCase.NwbObject, nwbFilePath), 'NWB:CustomConstraintUnfulfilled') end end diff --git a/+types/+core/ImageSeries.m b/+types/+core/ImageSeries.m index 8e5addb3..1fbdd219 100644 --- a/+types/+core/ImageSeries.m +++ b/+types/+core/ImageSeries.m @@ -186,6 +186,12 @@ end end end + %% CUSTOM CONSTRAINTS + function checkCustomConstraint(obj) + if ~isempty(obj.external_file) && isempty(obj.data), ... + obj.data = nan(1,1,2); + end + end end end \ No newline at end of file diff --git a/+types/+core/TimeSeries.m b/+types/+core/TimeSeries.m index 2503b45b..5245289c 100644 --- a/+types/+core/TimeSeries.m +++ b/+types/+core/TimeSeries.m @@ -415,6 +415,15 @@ io.writeAttribute(fid, [fullpath '/timestamps/unit'], obj.timestamps_unit); end end + %% CUSTOM CONSTRAINTS + function checkCustomConstraint(obj) + assert(~isempty(obj.timestamps) || ~isempty(obj.starting_time), ... + "'timestamps' or 'starting_time' must be specified") + if ~isempty(obj.starting_time) + assert(~isempty(obj.starting_time_rate), ... + "'starting_time_rate' must be specified when 'starting_time' is specified") + end + end end end \ No newline at end of file diff --git a/+types/+untyped/MetaClass.m b/+types/+untyped/MetaClass.m index eeffa248..2a1d04b9 100644 --- a/+types/+untyped/MetaClass.m +++ b/+types/+untyped/MetaClass.m @@ -1,8 +1,12 @@ -classdef MetaClass < handle +classdef MetaClass < handle & matlab.mixin.CustomDisplay properties (Hidden, SetAccess = private) metaClass_fullPath; end - + + properties (Constant, Transient, Access = protected) + REQUIRED containers.Map = containers.Map + end + methods function obj = MetaClass(varargin) end @@ -43,6 +47,11 @@ methods function refs = export(obj, fid, fullpath, refs) + % throwErrorIfCustomConstraintUnfulfilled is intentionally placed + % before throwErrorIfMissingRequiredProps. + % See file.fillCustomConstraint + obj.throwErrorIfCustomConstraintUnfulfilled(fullpath) + obj.throwErrorIfMissingRequiredProps(fullpath) obj.metaClass_fullPath = fullpath; %find reference properties propnames = properties(obj); @@ -107,4 +116,100 @@ function warnIfPropertyAttributeNotExported(obj, propName, depPropName, fullpath warning(warningId, warningMessage) %#ok end end -end \ No newline at end of file + + methods (Access = protected) % Override matlab.mixin.CustomDisplay + function str = getFooter(obj) + obj.displayWarningIfMissingRequiredProps(); + str = ''; + end + end + + methods (Access = protected) + function missingRequiredProps = checkRequiredProps(obj) + missingRequiredProps = {}; + requiredProps = obj.getRequiredProperties(); + + for i = 1:numel(requiredProps) + thisPropName = requiredProps{i}; + if isempty(obj.(thisPropName)) + missingRequiredProps{end+1} = thisPropName; %#ok + end + end + end + + function displayWarningIfMissingRequiredProps(obj) + missingRequiredProps = obj.checkRequiredProps(); + + % Exception: 'file_create_date' is automatically added by the + % matnwb API on export, so no need to warn if it is missing. + if isa(obj, 'types.core.NWBFile') + missingRequiredProps = setdiff(missingRequiredProps, 'file_create_date', 'stable'); + end + + % Exception: 'id' of DynamicTable is automatically assigned if not + % specified, so no need to warn if it is missing. + if isa(obj, 'types.hdmf_common.DynamicTable') + missingRequiredProps = setdiff(missingRequiredProps, 'id', 'stable'); + end + + if ~isempty( missingRequiredProps ) + warnState = warning('backtrace', 'off'); + cleanerObj = onCleanup(@(s) warning(warnState)); + + propertyListStr = obj.prettyPrintPropertyList(missingRequiredProps); + warning('NWB:RequiredPropertyMissing', ... + 'The following required properties are missing for instance for type "%s":\n%s', class(obj), propertyListStr) + end + end + + function throwErrorIfMissingRequiredProps(obj, fullpath) + missingRequiredProps = obj.checkRequiredProps(); + if ~isempty( missingRequiredProps ) + propertyListStr = obj.prettyPrintPropertyList(missingRequiredProps); + error('NWB:RequiredPropertyMissing', ... + 'The following required properties are missing for instance for type "%s" at file location "%s":\n%s', class(obj), fullpath, propertyListStr) + end + end + + function throwErrorIfCustomConstraintUnfulfilled(obj, fullpath) + try + obj.checkCustomConstraint() + catch ME + error('NWB:CustomConstraintUnfulfilled', ... + 'The following error was caught when exporting type "%s" at file location "%s":\n%s', class(obj), fullpath, ME.message) + end + end + end + + methods + function checkCustomConstraint(obj) %#ok + % This method is meant to be overridden by subclasses that have + % custom constraints that can not be inferred from the nwb schema. + end + end + + methods (Access = private) + function requiredProps = getRequiredProperties(obj) + + % Introspectively retrieve required properties and add to + % persistent map. + + if isKey(obj.REQUIRED, class(obj) ) + requiredProps = obj.REQUIRED( class(obj) ); + else + mc = metaclass(obj); + propertyDescription = {mc.PropertyList.Description}; + isRequired = startsWith(propertyDescription, 'REQUIRED'); + requiredProps = {mc.PropertyList(isRequired).Name}; + obj.REQUIRED( class(obj) ) = requiredProps; + end + end + end + + methods (Static, Access = private) + function propertyListStr = prettyPrintPropertyList(propertyNames) + propertyListStr = compose(' %s', string(propertyNames)); + propertyListStr = strjoin(propertyListStr, newline); + end + end +end diff --git a/.github/workflows/run_codespell.yml b/.github/workflows/run_codespell.yml index 127857ba..9a291110 100644 --- a/.github/workflows/run_codespell.yml +++ b/.github/workflows/run_codespell.yml @@ -12,7 +12,7 @@ on: branches: - master push: - branches-ignore: + branches: - master jobs: diff --git a/.github/workflows/run_tests.yml b/.github/workflows/run_tests.yml index 023ee852..0d6230c5 100644 --- a/.github/workflows/run_tests.yml +++ b/.github/workflows/run_tests.yml @@ -18,65 +18,67 @@ jobs: name: Run MATLAB tests runs-on: ubuntu-latest steps: - - name: check out repository + - name: Check out repository uses: actions/checkout@v4 - - name: install python + - name: Install python uses: actions/setup-python@v5 with: python-version: '3.10' - - name: configure python env + - name: Configure python env run: | python -m pip install -U pip pip install -r +tests/requirements.txt echo "HDF5_PLUGIN_PATH=$(python -c "import hdf5plugin; print(hdf5plugin.PLUGINS_PATH)")" >> "$GITHUB_ENV" - - name: install MATLAB + - name: Install MATLAB uses: matlab-actions/setup-matlab@v2 with: release: R2024a # this is necessary to test dynamic filters - - name: run tests + - name: Run tests uses: matlab-actions/run-command@v2 with: command: results = assertSuccess(nwbtest); assert(~isempty(results), 'No tests ran'); - - name: upload JUnit results + - name: Upload JUnit results + if: always() uses: actions/upload-artifact@v4 with: name: test-results path: testResults.xml retention-days: 1 - - name: upload coverage results + - name: Upload coverage results + if: always() uses: actions/upload-artifact@v4 with: name: test-coverage path: ./coverage.xml publish_junit: - name: Publish JUnit Test Results + name: Publish JUnit test results runs-on: ubuntu-latest - if: ${{ always() }} + if: always() needs: [run_tests] steps: - - name: retrieve result files + - name: Retrieve result files uses: actions/download-artifact@v4 with: name: test-results - - name: publish results + - name: Publish test results uses: mikepenz/action-junit-report@v4 with: report_paths: 'testResults.xml' publish_coverage: - name: Publish Cobertura Test Coverage + name: Publish Cobertura test coverage runs-on: ubuntu-latest needs: [run_tests] steps: - - name: check out repository + - name: Check out repository uses: actions/checkout@v4 - name: retrieve code coverage files uses: actions/download-artifact@v4 with: name: test-coverage - - name: publish on Codecov + - name: Publish on coverage results on Codecov uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} files: coverage.xml name: codecov-matnwb - verbose: true \ No newline at end of file + verbose: true diff --git a/nwbtest.m b/nwbtest.m index ba83e7d2..18a5f86a 100644 --- a/nwbtest.m +++ b/nwbtest.m @@ -62,7 +62,7 @@ coverageFile = fullfile(ws, 'coverage.xml'); [installDir, ~, ~] = fileparts(mfilename('fullpath')); - ignoreFolders = {'tutorials', '+contrib', '+util', 'external_packages', '+tests'}; + ignoreFolders = {'tutorials', 'tools', '+contrib', '+util', 'external_packages', '+tests'}; ignorePaths = {fullfile('+misc', 'generateDocs.m'), [mfilename '.m'], 'nwbClearGenerated.m'}; mfilePaths = getMfilePaths(installDir, ignoreFolders, ignorePaths); if ~verLessThan('matlab', '9.3') && ~isempty(mfilePaths)