From bb9acbf74c5cc67545a505edad2a0b46694a2b57 Mon Sep 17 00:00:00 2001 From: ehennestad Date: Thu, 12 Dec 2024 12:17:26 +0100 Subject: [PATCH] Improve error message if adding neurodata types of the wrong types as property values (#638) * Add utility methods for checking if an object / classname represents a neurodata type * Fix bug and improve error for invalid neurodata types in types.util.checkDType * Update isNeurodataTypeClassName.m Fix missing arg * Try running tests with pynwb pre-release (dev) * Add new error id to whitelist * Update run_tests.yml * Update requirements to use dev from pynwb and nwbinspector * Change error id in types.util.checkDType * Fix errorID in linkTest * Add test for new utility function * Fix +matnwb/+utility/isNeurodataTypeClassName.m Make check more robust --- +matnwb/+utility/isNeurodataType.m | 16 ++++++ +matnwb/+utility/isNeurodataTypeClassName.m | 58 +++++++++++++++++++++ +tests/+unit/FunctionTests.m | 7 +++ +tests/+unit/linkTest.m | 4 +- +tests/requirements.txt | 2 +- +types/+util/checkConstraint.m | 1 + +types/+util/checkDtype.m | 15 ++++-- 7 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 +matnwb/+utility/isNeurodataType.m create mode 100644 +matnwb/+utility/isNeurodataTypeClassName.m diff --git a/+matnwb/+utility/isNeurodataType.m b/+matnwb/+utility/isNeurodataType.m new file mode 100644 index 00000000..8f2af154 --- /dev/null +++ b/+matnwb/+utility/isNeurodataType.m @@ -0,0 +1,16 @@ +function tf = isNeurodataType(value) +% isNeurodataType - Check if a value / object is a neurodata type. +% +% tf = matnwb.utility.isNeurodataType(value) returns true if the value +% is an object of a class representing a neurodata type of the NWB Format. +% If the input is a string representing the class name of a neurodata +% type, the function will also return true. + + tf = false; + if isa(value, 'char') || isa(value, 'string') + tf = matnwb.utility.isNeurodataTypeClassName(value); + elseif isa(value, 'types.untyped.MetaClass') + className = class(value); + tf = matnwb.utility.isNeurodataTypeClassName(className); + end +end diff --git a/+matnwb/+utility/isNeurodataTypeClassName.m b/+matnwb/+utility/isNeurodataTypeClassName.m new file mode 100644 index 00000000..69f61cd8 --- /dev/null +++ b/+matnwb/+utility/isNeurodataTypeClassName.m @@ -0,0 +1,58 @@ +function tf = isNeurodataTypeClassName(typeName) +% isNeurodataTypeClassName - Check if a name is the class name of a neurodata type. +% +% tf = matnwb.utility.isNeurodataTypeClassName(value) returns true if a +% string is the class name of a class representing a neurodata type of +% the NWB Format + + arguments + typeName (1,1) string + end + + tf = false; + if startsWith(typeName, 'types.') && ~startsWith(typeName, 'types.untyped') + mc = meta.class.fromName(typeName); + if ~isempty(mc) + tf = hasSuperClass(mc, 'types.untyped.MetaClass'); + end + end +end + +function tf = hasSuperClass(mc, superClassName) +% hasSuperClass - Recursively check if a meta.class object has a specific superclass. +% +% tf = hasSuperClass(mc, superClassName) returns true if the meta.class object +% mc has a superclass with the name superClassName, either directly or +% indirectly (through its own superclasses). +% +% Arguments: +% mc - A meta.class object. +% superClassName - The name of the superclass to check for (string). +% +% Returns: +% tf - Logical value indicating if the class has the specified superclass. + + arguments + mc meta.class + superClassName (1,1) string + end + + % Check if the current class has the desired superclass directly. + for i = 1:numel(mc.SuperclassList) + if mc.SuperclassList(i).Name == superClassName + tf = true; + return; + end + end + + % If not, check recursively through each superclass. + for i = 1:numel(mc.SuperclassList) + if hasSuperClass(mc.SuperclassList(i), superClassName) + tf = true; + return; + end + end + + % If no match found, return false. + tf = false; +end \ No newline at end of file diff --git a/+tests/+unit/FunctionTests.m b/+tests/+unit/FunctionTests.m index 4b16b445..2e739c5c 100644 --- a/+tests/+unit/FunctionTests.m +++ b/+tests/+unit/FunctionTests.m @@ -34,5 +34,12 @@ function testWriteCompoundScalar(testCase) io.writeCompound(fid, '/map_data', data) H5F.close(fid); end + function testIsNeurodatatype(testCase) + timeSeries = types.core.TimeSeries(); + testCase.verifyTrue(matnwb.utility.isNeurodataType(timeSeries)) + + dataPipe = types.untyped.DataPipe('data', rand(10,10)); + testCase.verifyFalse(matnwb.utility.isNeurodataType(dataPipe)) + end end end \ No newline at end of file diff --git a/+tests/+unit/linkTest.m b/+tests/+unit/linkTest.m index 05f462f3..986f9665 100644 --- a/+tests/+unit/linkTest.m +++ b/+tests/+unit/linkTest.m @@ -100,7 +100,7 @@ function testDirectTypeAssignmentToSoftLinkProperty(testCase) end function testWrongTypeInSoftLinkAssignment(testCase) - + % Adding an OpticalChannel as device for ElectrodeGroup should fail. function createElectrodeGroupWithWrongDeviceType() not_a_device = types.core.OpticalChannel('description', 'test_channel'); electrodeGroup = types.core.ElectrodeGroup(... @@ -108,5 +108,5 @@ function createElectrodeGroupWithWrongDeviceType() 'device', not_a_device); %#ok end testCase.verifyError(@createElectrodeGroupWithWrongDeviceType, ... - 'NWB:TypeCorrection:InvalidConversion') + 'NWB:CheckDType:InvalidNeurodataType') end diff --git a/+tests/requirements.txt b/+tests/requirements.txt index 12070c83..c0038019 100644 --- a/+tests/requirements.txt +++ b/+tests/requirements.txt @@ -1,3 +1,3 @@ hdf5plugin git+https://github.com/NeurodataWithoutBorders/nwbinspector.git@dev -git+https://github.com/NeurodataWithoutBorders/pynwb.git@dev \ No newline at end of file +git+https://github.com/NeurodataWithoutBorders/pynwb.git@dev diff --git a/+types/+util/checkConstraint.m b/+types/+util/checkConstraint.m index 92b58324..bad61857 100644 --- a/+types/+util/checkConstraint.m +++ b/+types/+util/checkConstraint.m @@ -18,6 +18,7 @@ function checkConstraint(pname, name, namedprops, constrained, val) expectedErrorTypes = {... 'NWB:CheckDType:InvalidType', ... 'NWB:CheckDType:InvalidShape', ... + 'NWB:CheckDType:InvalidNeurodataType', ... 'NWB:TypeCorrection:InvalidConversion'}; if ~any(strcmp(ME.identifier, expectedErrorTypes)) rethrow(ME); diff --git a/+types/+util/checkDtype.m b/+types/+util/checkDtype.m index 8182e9aa..e02a1b09 100644 --- a/+types/+util/checkDtype.m +++ b/+types/+util/checkDtype.m @@ -57,11 +57,12 @@ 'Number of elements for each struct field must match to be valid.'], ... num2str(fieldSizes)); end - + + parentName = name; for iField = 1:length(expectedFields) % validate subfield types. name = expectedFields{iField}; - subName = [name '.' name]; + subName = [parentName '.' name]; subType = typeDescriptor.(name); if (isstruct(value) && isscalar(value)) || istable(value) @@ -123,7 +124,15 @@ value = unwrapValue(value); end -correctedValue = types.util.correctType(value, typeDescriptor); +if matnwb.utility.isNeurodataType(typeDescriptor) + errorId = 'NWB:CheckDType:InvalidNeurodataType'; + errorMessage = sprintf(['Expected value for `%s` to be of ', ... + 'type `%s`. Instead it was `%s`'], name, typeDescriptor, class(value)); + assert(isa(value, typeDescriptor), errorId, errorMessage) + correctedValue = value; +else + correctedValue = types.util.correctType(value, typeDescriptor); +end % this specific conversion is fine as HDF5 doesn't have a representative % datetime type. Thus we suppress the warning for this case. isDatetimeConversion = isa(correctedValue, 'datetime')...