From 7dd524db7627dc9cd7c205aea5b6708c8c22ba38 Mon Sep 17 00:00:00 2001 From: ehennestad Date: Thu, 1 Oct 2026 20:15:58 +0200 Subject: [PATCH 1/2] fix(types): allow one object in several unnamed groups HasUnnamedGroups exposes each entry of a type's unnamed groups as a property with the entry's name, and raised DuplicateEntry whenever a name appeared in more than one group. An object whose type matches more than one group is placed in each of them: an OpticalChannelPlus in an ndx-multichannel-volume ImagingVolume fills both the inherited optical channel group and the extension's own group, as it does in PyNWB. Creating such an object, and so reading any file with this layout, failed. A name may now appear in several groups when every occurrence is the same object. It has one location in the file and is exported once, so it gets one property. Different objects with the same name are still rejected, because both would be written to the same location. The property reads from any group that holds the name, assigning to it updates every group that accepts the value, remove() removes the name from all groups, and the property is deleted only when no group holds the name. Before, rejecting a duplicate also deleted the property of the entry that was already there. Co-Authored-By: Claude Opus 5.5 --- +matnwb/+mixin/HasUnnamedGroups.m | 119 +++++++++++++---- +tests/+unit/+schema/SharedEntryTest.m | 124 ++++++++++++++++++ .../sharedEntrySchema/ses.namespace.yaml | 6 + .../sharedEntrySchema/ses.types.yaml | 11 ++ 4 files changed, 237 insertions(+), 23 deletions(-) create mode 100644 +tests/+unit/+schema/SharedEntryTest.m create mode 100644 +tests/test-schema/sharedEntrySchema/ses.namespace.yaml create mode 100644 +tests/test-schema/sharedEntrySchema/ses.types.yaml diff --git a/+matnwb/+mixin/HasUnnamedGroups.m b/+matnwb/+mixin/HasUnnamedGroups.m index 35d1a8caa..b776f116c 100644 --- a/+matnwb/+mixin/HasUnnamedGroups.m +++ b/+matnwb/+mixin/HasUnnamedGroups.m @@ -16,6 +16,20 @@ % 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 +% and is written once, so 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 +145,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 +156,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) @@ -383,25 +400,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, or add the same object.'], ... + 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 +457,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 +486,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 +540,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..7ebc2dc65 --- /dev/null +++ b/+tests/+unit/+schema/SharedEntryTest.m @@ -0,0 +1,124 @@ +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 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 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: '*' From 67628bc0c7f560f1259a57b903d35bfeeaed785c Mon Sep 17 00:00:00 2001 From: ehennestad Date: Tue, 6 Oct 2026 12:04:23 +0200 Subject: [PATCH 2/2] fix(types): count shared entries once and clarify shared-entry messages - numEntries counts the unique entry names across all unnamed groups instead of summing each Set's Count, so an object held by two groups under one name is one entry. The count appears only in the display header of container-like types. ImagingPlaneWithSubtypeChannels is not container-like, and no public method reaches numEntries, so no test covers the shared case. HasUnnamedGroupsTest/testObjectDisplay still goes through the new code for a ProcessingModule. - The class help says that each group exports a shared object to the same location, so the file holds one copy. It no longer says that the object is written once. - The DuplicateEntry message ends with "Use a different name." add() rejects any existing name, so "add the same object" was not reachable through add(). - Add a test that assigning a value no group accepts raises NWB:Set:FailedValidation and leaves every group unchanged. Co-Authored-By: Claude Opus 5.5 --- +matnwb/+mixin/HasUnnamedGroups.m | 17 ++++++++++------- +tests/+unit/+schema/SharedEntryTest.m | 14 ++++++++++++++ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/+matnwb/+mixin/HasUnnamedGroups.m b/+matnwb/+mixin/HasUnnamedGroups.m index b776f116c..ac3eb6368 100644 --- a/+matnwb/+mixin/HasUnnamedGroups.m +++ b/+matnwb/+mixin/HasUnnamedGroups.m @@ -22,9 +22,10 @@ % - 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 -% and is written once, so it gets one property, and assigning to that -% property updates every group that holds the name. +% 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. @@ -294,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) @@ -412,7 +415,7 @@ function createDynamicProperty(obj, name, groupName) '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, or add the same object.'], ... + 'name.'], ... name, groupName) end diff --git a/+tests/+unit/+schema/SharedEntryTest.m b/+tests/+unit/+schema/SharedEntryTest.m index 7ebc2dc65..78be51776 100644 --- a/+tests/+unit/+schema/SharedEntryTest.m +++ b/+tests/+unit/+schema/SharedEntryTest.m @@ -71,6 +71,16 @@ function testAssigningUpdatesEveryGroupThatAcceptsValue(testCase) 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(); @@ -122,3 +132,7 @@ function testRoundTripWritesSharedEntryOnce(testCase) end end end + +function setGFP(plane, value) + plane.GFP = value; +end