fix(types): allow one object in several unnamed groups

+matnwb/+mixin/HasUnnamedGroups.m

@@ -16,6 +16,21 @@
 %   This mixin lets you write
 %     module.add('MyData', dataObject)
 %
+% Names of entries:
+%   Each entry is exposed as one property with the entry's name, so a name
+%   must identify a single object. The rules are:
+%   - A name may appear in several groups if every occurrence is the same
+%     object. This happens when an object's type matches more than one
+%     group, for example an entry of a subtype that also satisfies a group
+%     inherited from a parent type. The object has one location in the
+%     file: each group exports it to that same location, so the file holds
+%     one copy. It gets one property, and assigning to that property
+%     updates every group that holds the name.
+%   - Different objects may not share a name, because both would be
+%     written to the same location in the file. Adding one raises
+%     NWB:HasUnnamedGroups:DuplicateEntry and leaves it out of the group.
+%   - The property is removed when no group holds the name any more.
+%
 % Implementation details:
 %   - Data elements are added to objects of this class as dynamic properties.
 %   - Assign callback functions on Set object to make sure objects of this
@@ -131,6 +146,8 @@ function add(obj, name, value)
 
         function remove(obj, name)
         % remove - remove data object given it's original (actual) name
+        %
+        % An object held by several groups is removed from all of them.
 
             arguments
                 obj (1,1) matnwb.mixin.HasUnnamedGroups
@@ -140,23 +157,24 @@ function remove(obj, name)
             warnState = warning('off', 'NWB:Set:PropertyNameExistsForEntry');
             warningCleanup = onCleanup(@() warning(warnState));
 
+            wasRemoved = false;
             for groupName = obj.GroupPropertyNames
                 currentSet = obj.(groupName);
 
                 if isa(currentSet, 'types.untyped.Anon')
                     error('Not implemented yet')
                 end
-                
+
                 % Remove data entry if the name exists in this set
                 if currentSet.isKey(name)
                     currentSet.remove(name);
-                    return
-                else
-                    continue
+                    wasRemoved = true;
                 end
             end
 
-            obj.warnIfNameIsPropertyName(name)
+            if ~wasRemoved
+                obj.warnIfNameIsPropertyName(name)
+            end
         end
 
         function T = getAliasMap(obj)
@@ -277,13 +295,15 @@ function assignContainedSetCallbackFunctions(obj)
         end
 
         function n = numEntries(obj)
-            nPerGroup = zeros(1, numel(obj.GroupPropertyNames));
+            % Count names, not group memberships: an object held by several
+            % groups under one name is one entry.
+            namesPerGroup = cell(1, numel(obj.GroupPropertyNames));
 
             for i = 1:numel(obj.GroupPropertyNames)
                 groupName = obj.GroupPropertyNames(i);
-                nPerGroup(i) = obj.(groupName).Count;
+                namesPerGroup{i} = string(obj.(groupName).keys());
             end
-            n = sum(nPerGroup);
+            n = numel(unique([namesPerGroup{:}]));
         end
 
         function nonDynamicProperties = getNonDynamicProperties(obj)
@@ -383,25 +403,35 @@ function createDynamicProperty(obj, name, groupName)
                 validName = sprintf('%s_', name);
             end
 
-            % Verify that name only exists in one group
+            % A name may appear in several groups only if every occurrence
+            % is the same object (see the class help for the rules).
             nameCount = obj.countInstancesOfName(name);
             if nameCount > 1
-                setObj.remove(name)
-
-                error('NWB:HasUnnamedGroups:DuplicateEntry', ...
-                    ['An entry with name `%s` was detected in multiple ', ...
-                    'contained groups. Removed entry from group `%s`.'], ...
-                    name, groupName)
+                if ~obj.isSameObjectInAllGroups(name)
+                    setObj.remove(name)
+
+                    error('NWB:HasUnnamedGroups:DuplicateEntry', ...
+                        ['A different object with name `%s` already exists ', ...
+                        'in another group. Objects with the same name would ', ...
+                        'be written to the same location in the file, so the ', ...
+                        'entry was removed from group `%s`. Use a different ', ...
+                        'name.'], ...
+                        name, groupName)
+                end
+
+                if obj.PropertyManager.existOriginalName(name)
+                    return % The property was created for another group.
+                end
             end
-            
+
             % Ensure that property does not already exist.
             assert(~isprop(obj, validName), ...
                 'NWB:HasUnnamedGroups:DynamicPropertyExists', ...
                 'Property with name "%s" already exists', validName)
 
             % Create a getter method that will retrieve the value from the Set
-            getMethod = @(~) obj.getDynamicPropertyValueFromSet(name, groupName);
-            setMethod = @(nm, value, gNnm) obj.setDynamicPropertyValueToSet(name, value, groupName);
+            getMethod = @(~) obj.getDynamicPropertyValueFromSet(name);
+            setMethod = @(nm, value, gNnm) obj.setDynamicPropertyValueToSet(name, value);
 
             % Add the property using the PropertyManager
             obj.PropertyManager.addProperty(name, ...
@@ -430,6 +460,20 @@ function deleteDynamicProperty(obj, name)
             end
         end
 
+        function tf = isSameObjectInAllGroups(obj, name)
+        % isSameObjectInAllGroups - Check that every group holding name holds the same object
+            entries = {};
+            for groupName = obj.GroupPropertyNames
+                containerObj = obj.(groupName);
+                if containerObj.isKey(name)
+                    entries{end+1} = containerObj.get(name); %#ok<AGROW>
+                end
+            end
+            % Identity, not equality: equal but separate objects would
+            % still be written twice to the same location.
+            tf = all(cellfun(@(entry) isa(entry, 'handle') && entry == entries{1}, entries));
+        end
+
         function result = getClassNamesForAllowedGroupTypes(obj)
         % getAllowedGroupTypes - Resolve full class names for the allowed group types.
             groupPropertyNames = obj.GroupPropertyNames;
@@ -445,14 +489,44 @@ function deleteDynamicProperty(obj, name)
 
     % Dynamic property getter methods
     methods (Access = private)
-        function value = getDynamicPropertyValueFromSet(obj, name, groupName)
-            % Get the value from the Set
-            value = obj.(groupName).get(name);
+        function value = getDynamicPropertyValueFromSet(obj, name)
+            % Get the value from the first Set that holds the name. Every
+            % Set that holds it holds the same object.
+            for groupName = obj.GroupPropertyNames
+                if obj.(groupName).isKey(name)
+                    value = obj.(groupName).get(name);
+                    return
+                end
+            end
         end
-
-        function value = setDynamicPropertyValueToSet(obj, name, value, groupName)
-            % Set the value to the Set of the contained subgroup
-            obj.(groupName).set(name, value);
+
+        function value = setDynamicPropertyValueToSet(obj, name, value)
+            % Set the value in every Set that holds the name, so they keep
+            % holding the same object. A Set whose type constraint the new
+            % value does not meet no longer holds the name.
+            groupsHoldingName = obj.GroupPropertyNames( ...
+                arrayfun(@(groupName) obj.(groupName).isKey(name), obj.GroupPropertyNames));
+
+            isAccepted = false(size(groupsHoldingName));
+            for iGroup = 1:numel(groupsHoldingName)
+                try
+                    obj.(groupsHoldingName(iGroup)).set(name, value, ...
+                        'FailOnInvalidType', true);
+                    isAccepted(iGroup) = true;
+                catch ME
+                    if ~strcmp(ME.identifier, 'NWB:Set:FailedValidation')
+                        rethrow(ME)
+                    end
+                    validationError = ME;
+                end
+            end
+
+            if ~any(isAccepted)
+                throw(validationError)
+            end
+            for groupName = groupsHoldingName(~isAccepted)
+                obj.(groupName).remove(name);
+            end
         end
 
         function value = getDynamicPropertyValueFromAnon(obj, groupName)
@@ -469,7 +543,9 @@ function onSetEntryAdded(obj, name, groupName)
 
         function onSetEntryRemoved(obj, name, ~)
         % onSetEntryRemoved - Handle entries being removed from a contained Set
-            obj.deleteDynamicProperty(name)
+            if ~obj.nameExists(name)
+                obj.deleteDynamicProperty(name)
+            end
         end
     end
 

+tests/+unit/+schema/SharedEntryTest.m

@@ -0,0 +1,138 @@
+classdef SharedEntryTest < tests.unit.abstract.SchemaTest
+% SharedEntryTest - Tests for an object held by two unnamed groups.
+%
+% ImagingPlaneWithSubtypeChannels inherits the opticalchannel group
+% (OpticalChannel) from ImagingPlane and adds the opticalchannelsubtype
+% group (OpticalChannelSubtype, a subtype of OpticalChannel). An
+% OpticalChannelSubtype entry matches both groups.
+
+    properties (Constant)
+        SchemaFolder = "sharedEntrySchema"
+        SchemaNamespaceFileName = "ses.namespace.yaml"
+    end
+
+    methods (TestMethodSetup)
+        function setupMethod(testCase)
+            testCase.applyFixture(matlab.unittest.fixtures.WorkingFolderFixture);
+        end
+    end
+
+    methods (Test)
+        function testSubtypeEntryIsHeldByBothGroups(testCase)
+            channel = testCase.createSubtypeChannel();
+            plane = testCase.createPlane('GFP', channel);
+
+            testCase.verifySameHandle(plane.opticalchannel.get('GFP'), channel)
+            testCase.verifySameHandle(plane.opticalchannelsubtype.get('GFP'), channel)
+            testCase.verifySameHandle(plane.GFP, channel)
+        end
+
+        function testDifferentObjectWithExistingNameErrors(testCase)
+            plane = testCase.createPlane('Channel', testCase.createChannel());
+
+            testCase.verifyError( ...
+                @() plane.opticalchannelsubtype.set('Channel', testCase.createSubtypeChannel()), ...
+                'NWB:HasUnnamedGroups:DuplicateEntry')
+            testCase.verifyFalse(plane.opticalchannelsubtype.isKey('Channel'))
+            testCase.verifyClass(plane.Channel, 'types.core.OpticalChannel')
+        end
+
+        function testRemoveDeletesSharedEntryFromAllGroups(testCase)
+            plane = testCase.createPlane('GFP', testCase.createSubtypeChannel());
+
+            plane.remove('GFP')
+
+            testCase.verifyFalse(plane.opticalchannel.isKey('GFP'))
+            testCase.verifyFalse(plane.opticalchannelsubtype.isKey('GFP'))
+            testCase.verifyFalse(isprop(plane, 'GFP'))
+        end
+
+        function testPropertyRemainsWhileAnyGroupHoldsName(testCase)
+            channel = testCase.createSubtypeChannel();
+            plane = testCase.createPlane('GFP', channel);
+
+            plane.opticalchannelsubtype.remove('GFP')
+
+            testCase.verifySameHandle(plane.GFP, channel)
+        end
+
+        function testAssigningUpdatesEveryGroupThatAcceptsValue(testCase)
+            plane = testCase.createPlane('GFP', testCase.createSubtypeChannel());
+
+            newSubtypeChannel = testCase.createSubtypeChannel();
+            plane.GFP = newSubtypeChannel;
+            testCase.verifySameHandle(plane.opticalchannel.get('GFP'), newSubtypeChannel)
+            testCase.verifySameHandle(plane.opticalchannelsubtype.get('GFP'), newSubtypeChannel)
+
+            % A plain OpticalChannel does not fit the subtype group.
+            newChannel = testCase.createChannel();
+            plane.GFP = newChannel;
+            testCase.verifySameHandle(plane.opticalchannel.get('GFP'), newChannel)
+            testCase.verifyFalse(plane.opticalchannelsubtype.isKey('GFP'))
+        end
+
+        function testAssigningValueNoGroupAcceptsErrors(testCase)
+            channel = testCase.createSubtypeChannel();
+            plane = testCase.createPlane('GFP', channel);
+
+            testCase.verifyError(@() setGFP(plane, types.core.Device()), ...
+                'NWB:Set:FailedValidation')
+            testCase.verifySameHandle(plane.opticalchannel.get('GFP'), channel)
+            testCase.verifySameHandle(plane.opticalchannelsubtype.get('GFP'), channel)
+        end
+
+        function testRoundTripWritesSharedEntryOnce(testCase)
+            device = types.core.Device();
+            channel = testCase.createSubtypeChannel();
+            plane = testCase.createPlane('GFP', channel, device);
+
+            nwb = NwbFile( ...
+                'identifier', 'SES', ...
+                'session_description', 'shared entry schema testing', ...
+                'session_start_time', datetime(2024, 1, 1, 'TimeZone', 'local'));
+            nwb.general_devices.set('Microscope', device);
+            nwb.general_optophysiology.set('Plane', plane);
+            nwbExport(nwb, 'testses.nwb');
+
+            planeInfo = h5info('testses.nwb', '/general/optophysiology/Plane');
+            testCase.verifyEqual({planeInfo.Groups.Name}, ...
+                {'/general/optophysiology/Plane/GFP'})
+
+            nwbIn = nwbRead('testses.nwb', 'ignorecache');
+            planeIn = nwbIn.general_optophysiology.get('Plane');
+            testCase.verifyClass(planeIn.GFP, 'types.ses.OpticalChannelSubtype')
+            testCase.verifySameHandle( ...
+                planeIn.opticalchannel.get('GFP'), planeIn.opticalchannelsubtype.get('GFP'))
+        end
+    end
+
+    methods (Static)
+        function plane = createPlane(channelName, channel, device)
+            if nargin < 3
+                device = types.core.Device();
+            end
+            plane = types.ses.ImagingPlaneWithSubtypeChannels( ...
+                'device', types.untyped.SoftLink(device), ...
+                'excitation_lambda', 488, ...
+                'indicator', 'GCaMP6f', ...
+                'location', 'V1', ...
+                channelName, channel);
+        end
+
+        function channel = createChannel()
+            channel = types.core.OpticalChannel( ...
+                'description', 'Green channel', ...
+                'emission_lambda', 525);
+        end
+
+        function channel = createSubtypeChannel()
+            channel = types.ses.OpticalChannelSubtype( ...
+                'description', 'Green channel', ...
+                'emission_lambda', 525);
+        end
+    end
+end
+
+function setGFP(plane, value)
+    plane.GFP = value;
+end

+tests/test-schema/sharedEntrySchema/ses.namespace.yaml

@@ -0,0 +1,6 @@
+namespaces:
+- full_name: Entries shared by an inherited group and a subtype group
+  name: ses
+  schema:
+  - namespace: core
+  - source: ses.types.yaml

+tests/test-schema/sharedEntrySchema/ses.types.yaml

@@ -0,0 +1,11 @@
+groups:
+- neurodata_type_def: OpticalChannelSubtype
+  neurodata_type_inc: OpticalChannel
+  doc: An optical channel subtype.
+- neurodata_type_def: ImagingPlaneWithSubtypeChannels
+  neurodata_type_inc: ImagingPlane
+  doc: An imaging plane with a group for optical channel subtypes, in addition to the inherited optical channel group.
+  groups:
+  - neurodata_type_inc: OpticalChannelSubtype
+    doc: Optical channels of the subtype.
+    quantity: '*'