diff --git a/+matnwb/+mixin/HasUnnamedGroups.m b/+matnwb/+mixin/HasUnnamedGroups.m index 35d1a8caa..ac3eb6368 100644 --- a/+matnwb/+mixin/HasUnnamedGroups.m +++ b/+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 + 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 diff --git a/+tests/+unit/+schema/SharedEntryTest.m b/+tests/+unit/+schema/SharedEntryTest.m new file mode 100644 index 000000000..78be51776 --- /dev/null +++ b/+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 diff --git a/+tests/test-schema/sharedEntrySchema/ses.namespace.yaml b/+tests/test-schema/sharedEntrySchema/ses.namespace.yaml new file mode 100644 index 000000000..7491d0ed4 --- /dev/null +++ b/+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 diff --git a/+tests/test-schema/sharedEntrySchema/ses.types.yaml b/+tests/test-schema/sharedEntrySchema/ses.types.yaml new file mode 100644 index 000000000..3d377dbd6 --- /dev/null +++ b/+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: '*'