Fix dataset.data validation (#742)

* Fix dataset/data validation

* Reset default assumption to scalar

* Fix expansion of inherited keys for dataset classes

* Regenerate type classes

* Updated images tutorial

Standardised variable names
Added more comments about the image data permutation
Added data types created in the last section to the nwb file
Permuted the rgb_street data for valid nwb shape

* Updated text sections of tutorial

* minor change

* Updated images tutorial

Minor changes

* Minor changes

* Fix some failling tests

* Update mss.multishapes.yaml

Reordered keys to follow recommended key order
Added shape constraint for NarrowInheritedDataset

* Update MultipleShapesTest.m

Added test to verify that shapes are correctly validated for inherited datasets of 'class_type' dataset

* Add adhoc fix for inconsistent schema shape specifications

* Additional fix for inconsistent schema specs

* Change unconstrained shape from any to null

* Cleanup and added comments

Author: ehennestad

+file/Dataset.m

@@ -150,7 +150,14 @@
                 );
 
             if ~isempty(obj.dtype)
-                props('data') = obj.dtype;
+                % Add a value to the props map representing the dataset
+                % itself. The prop name of a dataset class is always data,
+                % the type should be empty, and we add a custom doc.
+                objCopy = obj;
+                objCopy.name = 'data';
+                objCopy.type = ''; % Reset type, as this now represents a property
+                objCopy.doc = sprintf('Data property for dataset class (%s)', obj.type);
+                props('data') = objCopy;
             end
 
             if ~isempty(obj.attributes)

+file/fillValidators.m

@@ -187,6 +187,10 @@
 
 function fdvstr = fillDimensionValidation(name, shape)
 
+    if isnumeric(shape) && isnan(shape) % Any shape is allowed
+        fdvstr = ''; return
+    end
+
     if iscell(shape)
         if ~isempty(shape) && iscell(shape{1})
             for i = 1:length(shape)

+file/formatShape.m

@@ -1,5 +1,10 @@
 function sz = formatShape(shape)
 %check for optional dims
+
+if ~iscell(shape) && isnumeric(shape) && isnan(shape)
+    sz = shape; return
+end
+
 assert(iscell(shape), '`shape` must be a cell.');
 
 if isempty(shape)

+file/isShapeScalar.m

@@ -1,4 +1,5 @@
 function isScalar = isShapeScalar(shape)
+if isnumeric(shape) && isnan(shape); isScalar = false; return; end
 if ~iscell(shape)
     shape = {shape};
 elseif iscell(shape{1})

+file/private/applySchemaVersionPatches.m

@@ -0,0 +1,34 @@
+function classSpec = applySchemaVersionPatches(className, classSpec, propSpecs, namespaceInfo)
+% applySchemaVersionPatches - Applies patches to schema specifications for classes.
+
+    switch className
+        case 'ScratchData'
+            % Spec does not define a shape, so it defaults to scalar, but
+            % scratch data can be non-scalar
+            fixShapeForDataProp(propSpecs)
+
+        case {'VectorData', 'VectorIndex'} % NWB v2.0.2 - v2.2.1
+            % Spec for older NWB versions did not define a shape, so it 
+            % defaults to scalar, but these types support non-scalar data
+            if strcmp(namespaceInfo.name, 'core') && ...
+                    ismember(namespaceInfo.version, {'2.0.2', '2.1.0'})
+                fixShapeForDataProp(propSpecs)
+                classSpec = fixShapeForDataClass(classSpec);
+            elseif strcmp(namespaceInfo.name, 'hdmf_common') && ...
+                    ismember(namespaceInfo.version, {'1.1.0', '1.1.2'})
+                fixShapeForDataProp(propSpecs)
+                classSpec = fixShapeForDataClass(classSpec);
+            end
+    end
+end
+
+function fixShapeForDataProp(propSpecs)
+    dataProp = propSpecs('data');
+    dataProp.shape = nan;
+    dataProp.scalar = false;
+    propSpecs('data') = dataProp; %#ok<NASGU> Map is a handle object
+end
+function classSpec = fixShapeForDataClass(classSpec)
+    classSpec.shape = nan;
+    classSpec.scalar = false;
+end

+file/processClass.m

@@ -15,7 +15,7 @@
         nodename = node(TYPEDEF_KEYS{hasTypeDefs});
 
         if ~isKey(pregen, nodename)
-            
+
             spec.internal.resolveInheritedFields(node, branch(iAncestor+1:end))
 
             switch node('class_type')
@@ -32,6 +32,9 @@
             end
             props = class.getProps();
 
+            % Apply patches for special cases of schema/specification errors
+            class = applySchemaVersionPatches(nodename, class, props, namespace);
+
             pregen(nodename) = struct('class', class, 'props', props);
         end
         try

+spec/+internal/expandInheritedFields.m

@@ -0,0 +1,30 @@
+function expandInheritedFields(childSpec, ancestorSpec)
+% expandInheritedFields - Include specification keys from an ancestor
+% specification for keys that are missing in a child specification.
+
+    primitiveTypes = enumeration('spec.enum.Primitives');
+    primitiveTypeKeys = {primitiveTypes.Key};
+
+    ignoreKeys = { ...
+        'neurodata_type_def', ...
+        'data_type_def', ...
+        'neurodata_type_inc', ...
+        'data_type_inc'};
+
+    ancestorSpecKeys = ancestorSpec.keys();
+    for iKey = 1:numel(ancestorSpecKeys)
+        currentAncestorKey = ancestorSpecKeys{iKey};
+        if any(strcmp(currentAncestorKey, primitiveTypeKeys))
+            if isKey(childSpec, currentAncestorKey)
+                % Update specifications for nested HDMF primitive types
+                spec.internal.updateSpecFromAncestorSpec(...
+                    childSpec(currentAncestorKey), ...
+                    ancestorSpec(currentAncestorKey))
+            end
+        elseif any(strcmp(currentAncestorKey, ignoreKeys))
+            continue % skip ignore keys
+        elseif ~isKey(childSpec, currentAncestorKey)
+            childSpec(currentAncestorKey) = ancestorSpec(currentAncestorKey);
+        end
+    end
+end

+spec/+internal/resolveInheritedFields.m

@@ -15,11 +15,15 @@ function resolveInheritedFields(typeSpec, ancestorTypeSpecs)
     for i = 1:length(ancestorTypeSpecs)
         ancestorType = ancestorTypeSpecs{i};
 
-        for j = 1:numel(primitiveTypes)
-            primitiveKey = primitiveTypes(j).Key; % i.e: 'groups', 'datasets' etc.
-            if isKey(typeSpec, primitiveKey) && isKey(ancestorType, primitiveKey)
-                spec.internal.updateSpecFromAncestorSpec(...
-                    typeSpec(primitiveKey), ancestorType(primitiveKey))
+        if strcmp(typeSpec('class_type'), 'datasets')
+            spec.internal.expandInheritedFields(typeSpec, ancestorType)
+        else % (class_type: group)
+            for j = 1:numel(primitiveTypes)
+                primitiveKey = primitiveTypes(j).Key; % i.e: 'groups', 'datasets' etc.
+                if isKey(typeSpec, primitiveKey) && isKey(ancestorType, primitiveKey)
+                    spec.internal.updateSpecFromAncestorSpec(...
+                        typeSpec(primitiveKey), ancestorType(primitiveKey))
+                end
             end
         end
     end

+spec/+internal/updateSpecFromAncestorSpec.m

@@ -11,9 +11,6 @@ function updateSpecFromAncestorSpec(childSpecs, ancestorSpecs)
 % Output Arguments:
 %   None. The function modifies child specifications in place.
 
-    primitiveTypes = enumeration('spec.enum.Primitives');
-    primitiveTypeKeys = {primitiveTypes.Key};
-
     for iSpec = 1:numel(childSpecs)
         currentSpec = childSpecs{iSpec};
 
@@ -24,19 +21,6 @@ function updateSpecFromAncestorSpec(childSpecs, ancestorSpecs)
             continue
         end
 
-        ancestorSpecKeys = matchingAncestorSpec.keys();
-        for iKey = 1:numel(ancestorSpecKeys)
-            currentAncestorKey = ancestorSpecKeys{iKey};
-            if any(strcmp(currentAncestorKey, primitiveTypeKeys))
-                if isKey(currentSpec, currentAncestorKey)
-                    % Recursively update nested specification primitives
-                    spec.internal.updateSpecFromAncestorSpec(...
-                        currentSpec(currentAncestorKey), ...
-                        matchingAncestorSpec(currentAncestorKey))
-                end
-            elseif ~isKey(currentSpec, currentAncestorKey)
-                currentSpec(currentAncestorKey) = matchingAncestorSpec(currentAncestorKey);
-            end
-        end
+        spec.internal.expandInheritedFields(currentSpec, matchingAncestorSpec)
     end
 end

+tests/+unit/+schema/MultipleShapesTest.m

@@ -8,7 +8,7 @@
     methods (Test)
         function testMultipleShapesDataset(testCase)
             msd = types.mss.MultiShapeDataset('data', rand(3, 1));
-            msd.data = rand(1, 5, 7);
+            msd.data = rand(7, 5, 1); % NB: Reverse dimensions relative to schema due to MATLAB F-ordering
             testCase.roundabout(msd);
         end
 
@@ -17,7 +17,7 @@ function testNullShapeDataset(testCase)
             randiMax = intmax('int8') - 1;
             for i=1:100
                 %test validation
-                nsd.data = rand(randi(randiMax) + 1, 3);
+                nsd.data = rand(3, randi(randiMax) + 1); % NB: Reverse dimensions relative to schema due to MATLAB F-ordering
             end
             testCase.roundabout(nsd);
         end
@@ -40,6 +40,17 @@ function testInheritedDtypeDataset(testCase)
             nid.data = 'Inherited Dtype Dataset';
             testCase.roundabout(nid);
         end
+
+        function testInheritedDatasetWithWrongShape(testCase)
+
+            % Should not fail for parent type that accepts data with shape 3
+            types.mss.NullShapeDataset('data', {'A'; 'B'; 'C'});
+    
+            % Should fail for inherited type that accepts data with shape 1
+            testCase.verifyError(...
+                @() types.mss.NarrowInheritedDataset('data', {'A'; 'B'; 'C'}'), ...
+                'NWB:CheckDims:InvalidDimensions');
+        end
     end
 
     methods (Access = private)