Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: Add arguments block to main MatNWB api functions #619

Merged
merged 36 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
4a305d0
Refactor: Add arguments block to main MatNWB api functions
ehennestad Nov 7, 2024
0bec70d
Update functionSignatures
ehennestad Nov 7, 2024
895fa1f
Create functionSignatures.json
ehennestad Nov 7, 2024
39bb147
Improve coverage for main functions
ehennestad Nov 7, 2024
1c1d7ec
Create TypeConversionTest.m
ehennestad Nov 7, 2024
2a31310
Add unit tests for io.isBool and io.pathParts
ehennestad Nov 7, 2024
ff62e74
Update functionSignatures
ehennestad Nov 7, 2024
1b3da95
Merge branch 'master' into refactor/add-argument-blocks
ehennestad Nov 8, 2024
ef2c592
Add test for cloneNwbFileClass
ehennestad Nov 8, 2024
0c0671f
Create local validator function for reftype in file.fillProps
ehennestad Nov 8, 2024
a2ac9f0
Create local function for duplicated code block
ehennestad Nov 8, 2024
cd21682
Simplify file cleanup in case of error
ehennestad Nov 8, 2024
ef99eb9
Update writeNamespace.m
ehennestad Nov 8, 2024
a78a239
Fix failing tests
ehennestad Nov 8, 2024
29a3021
Improve coverage
ehennestad Nov 8, 2024
efafb53
Add tests for misc functions
ehennestad Nov 8, 2024
5429e56
Simplify spec.loadCache
ehennestad Nov 8, 2024
d92dacb
Remove unused function
ehennestad Nov 9, 2024
ffdc5b8
Add misc unittests
ehennestad Nov 9, 2024
79f2603
Add datapipe test and fix bug in DynamicFilter class
ehennestad Nov 10, 2024
d7b9edd
Add unittests for functions in +types namespace
ehennestad Nov 10, 2024
7469fa5
Add class setup to +types function tests
ehennestad Nov 10, 2024
0282719
Add tests for clearing dynamictable plus fix related bugs
ehennestad Nov 10, 2024
2f30b4d
Add input options for nwbClearGenerated
ehennestad Nov 10, 2024
6fc75ce
Add cleanup for writeAttribute
ehennestad Nov 11, 2024
53057ca
Update Point.m
ehennestad Nov 11, 2024
ac3b990
Add SpaceTest
ehennestad Nov 11, 2024
3aaed8f
Fix bug with writing and parsing logical data in compound data type
ehennestad Nov 11, 2024
0749a6a
Fix variableName in test
ehennestad Nov 11, 2024
3e2fa00
Merge branch 'fix/write-parse-compound-bug' into refactor/add-argumen…
ehennestad Nov 11, 2024
2b12acb
Add more unittests to WriteTest
ehennestad Nov 11, 2024
befd8fe
Remove unused function
ehennestad Nov 11, 2024
a6c61d4
Add unit tests for functions in +types namespace
ehennestad Nov 11, 2024
00d1623
Change exist to isfolder/isfile
ehennestad Nov 11, 2024
f97b6a9
Merge branch 'master' into refactor/add-argument-blocks
bendichter Nov 14, 2024
b18424a
Merge branch 'master' into refactor/add-argument-blocks
ehennestad Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions +file/cloneNwbFileClass.m
Copy link
Collaborator Author

@ehennestad ehennestad Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Rename variables for clearer code
  • Use fileread instead of fopen, fread, fclose sequence
  • Change name of superclass in call to superclass constructor in the new class def

Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,21 @@ function cloneNwbFileClass(typeFileName, fullTypeName)

nwbFilePath = which('NwbFile');
installPath = fileparts(nwbFilePath);
fileId = fopen(nwbFilePath);
text = strrep(char(fread(fileId) .'),...
'NwbFile < types.core.NWBFile',...
nwbFileClassDef = fileread(nwbFilePath);

% Update superclass name
updatedNwbFileClassDef = strrep(nwbFileClassDef, ...
'NwbFile < types.core.NWBFile', ...
sprintf('NwbFile < %s', fullTypeName));
fclose(fileId);

% Update call to superclass constructor
updatedNwbFileClassDef = strrep(updatedNwbFileClassDef, ...
'obj = obj@types.core.NWBFile', ...
sprintf('obj = obj@%s', fullTypeName));

fileId = fopen(fullfile(installPath, [typeFileName '.m']), 'W');
fwrite(fileId, text);
fwrite(fileId, updatedNwbFileClassDef);
fclose(fileId);
end

rehash();
end
10 changes: 3 additions & 7 deletions +file/fillExport.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Remove keyboard (debug statement)
  • Negate if statement to remove redundant return statement

Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@
for i = 1:length(propertyNames)
propertyName = propertyNames{i};
pathProps = traverseRaw(propertyName, RawClass);
if isempty(pathProps)
keyboard;
end
prop = pathProps{end};
elideProps = pathProps(1:end-1);
elisions = cell(length(elideProps),1);
Expand Down Expand Up @@ -84,11 +81,10 @@
path = {};

if isa(RawClass, 'file.Dataset')
if isempty(RawClass.attributes)
return;
if ~isempty(RawClass.attributes)
matchesAttribute = strcmp({RawClass.attributes.name}, propertyName);
path = {RawClass.attributes(matchesAttribute)};
end
matchesAttribute = strcmp({RawClass.attributes.name}, propertyName);
path = {RawClass.attributes(matchesAttribute)};
return;
end

Expand Down
40 changes: 20 additions & 20 deletions +file/fillProps.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Create function assertValidRefType to factor out duplicate code
  • Reformat typeStr with improved capitalisation

Original file line number Diff line number Diff line change
Expand Up @@ -52,30 +52,14 @@
typeStr = ['Table with columns: (', strjoin(columnDocStr, ', '), ')'];
elseif isa(prop, 'file.Attribute')
if isa(prop.dtype, 'containers.Map')
switch prop.dtype('reftype')
case 'region'
refTypeName = 'Region';
case 'object'
refTypeName = 'Object';
otherwise
error('NWB:ClassGenerator:InvalidRefType', ...
'Invalid reftype found while filling description for class property "%s".', propName);
end
typeStr = sprintf('%s Reference to %s', refTypeName, prop.dtype('target_type'));
assertValidRefType(prop.dtype('reftype'))
typeStr = sprintf('%s reference to %s', capitalize(prop.dtype('reftype')), prop.dtype('target_type'));
else
typeStr = prop.dtype;
end
elseif isa(prop, 'containers.Map')
switch prop('reftype')
case 'region'
refTypeName = 'region';
case 'object'
refTypeName = 'object';
otherwise
error('NWB:ClassGenerator:InvalidRefType', ...
'Invalid reftype found while filling description for class property "%s".', propName);
end
typeStr = sprintf('%s Reference to %s', refTypeName, prop('target_type'));
assertValidRefType(prop('reftype'))
typeStr = sprintf('%s reference to %s', capitalize(prop('reftype')), prop('target_type'));
elseif isa(prop, 'file.interface.HasProps')
typeStrCell = cell(size(prop));
for iProp = 1:length(typeStrCell)
Expand Down Expand Up @@ -108,4 +92,20 @@
if nargin >= 2
propStr = [propName ' = ' propStr];
end
end

function assertValidRefType(referenceType)
arguments
referenceType (1,1) string
end
assert( ismember(referenceType, ["region", "object"]), ...
'NWB:ClassGenerator:InvalidRefType', ...
'Invalid reftype found while filling description for class properties.')
end

function word = capitalize(word)
arguments
word (1,:) char
end
word(1) = upper(word(1));
end
44 changes: 22 additions & 22 deletions +file/fillValidators.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Create local function getFullClassName for duplicate code

Original file line number Diff line number Diff line change
Expand Up @@ -155,34 +155,19 @@
fillDimensionValidation(prop.dtype, prop.shape)...
}, newline);
elseif prop.isConstrainedSet
try
fullname = namespaceReg.getFullClassName(prop.type);
catch ME
if ~endsWith(ME.identifier, 'Namespace:NotFound')
rethrow(ME);
end

warning('NWB:Fill:Validators:NamespaceNotFound',...
['Namespace could not be found for type `%s`.' ...
' Skipping Validation for property `%s`.'], prop.type, name);
return;
fullname = getFullClassName(namespaceReg, prop.type, name);
if isempty(fullname)
return

Check warning on line 160 in +file/fillValidators.m

View check run for this annotation

Codecov / codecov/patch

+file/fillValidators.m#L160

Added line #L160 was not covered by tests
end

unitValidationStr = strjoin({unitValidationStr...
['constrained = { ''' fullname ''' };']...
['types.util.checkSet(''' name ''', struct(), constrained, val);']...
}, newline);
else
try
fullname = namespaceReg.getFullClassName(prop.type);
catch ME
if ~endsWith(ME.identifier, 'Namespace:NotFound')
rethrow(ME);
end

warning('NWB:Fill:Validators:NamespaceNotFound',...
['Namespace could not be found for type `%s`.' ...
' Skipping Validation for property `%s`.'], prop.type, name);
return;
fullname = getFullClassName(namespaceReg, prop.type, name);
if isempty(fullname)
return

Check warning on line 170 in +file/fillValidators.m

View check run for this annotation

Codecov / codecov/patch

+file/fillValidators.m#L170

Added line #L170 was not covered by tests
end
unitValidationStr = [unitValidationStr newline fillDtypeValidation(name, fullname)];
end
Expand Down Expand Up @@ -318,4 +303,19 @@
condition, ...
sprintf(' %s', errorStr), ...
'end' }, newline );
end

function fullname = getFullClassName(namespaceReg, propType, name)
fullname = '';
try
fullname = namespaceReg.getFullClassName(propType);
catch ME
if ~endsWith(ME.identifier, 'Namespace:NotFound')
rethrow(ME);

Check warning on line 314 in +file/fillValidators.m

View check run for this annotation

Codecov / codecov/patch

+file/fillValidators.m#L312-L314

Added lines #L312 - L314 were not covered by tests
end

warning('NWB:Fill:Validators:NamespaceNotFound',...
['Namespace could not be found for type `%s`.' ...
' Skipping Validation for property `%s`.'], propType, name);

Check warning on line 319 in +file/fillValidators.m

View check run for this annotation

Codecov / codecov/patch

+file/fillValidators.m#L317-L319

Added lines #L317 - L319 were not covered by tests
end
end
18 changes: 7 additions & 11 deletions +file/writeNamespace.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Replace exist(name, 'dir') with isfolder(name) for performance and clearer code
  • Negate if to remove unreachable lines
  • Use onCleanup to make sure file is closed on errors instead of using try/catch block.

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ function writeNamespace(namespaceName, saveDir)

classFileDir = fullfile(saveDir, '+types', ['+' misc.str2validName(Namespace.name)]);

if 7 ~= exist(classFileDir, 'dir')
if ~isfolder(classFileDir)
mkdir(classFileDir);
end

Expand All @@ -14,18 +14,14 @@ function writeNamespace(namespaceName, saveDir)
className = classes{i};
[processed, classprops, inherited] = file.processClass(className, Namespace, pregenerated);

if isempty(processed)
continue;
end

fid = fopen(fullfile(classFileDir, [className '.m']), 'W');
try
if ~isempty(processed)
fid = fopen(fullfile(classFileDir, [className '.m']), 'W');
% Create cleanup object to close to file in case the write operation fails.
fileCleanupObj = onCleanup(@(id) fclose(fid));
fwrite(fid, file.fillClass(className, Namespace, processed, ...
classprops, inherited), 'char');
catch ME
fclose(fid);
rethrow(ME)
else
% pass
end
fclose(fid);
end
end
6 changes: 2 additions & 4 deletions +io/+space/+shape/Point.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify

Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,9 @@
end

function varargout = getMatlabIndex(obj)
if 0 == nargout
return;
if nargout > 0
varargout{1} = obj.index;
end

varargout{1} = obj.index;
end
end
end
Expand Down
18 changes: 18 additions & 0 deletions +io/+spec/+internal/readEmbeddedSpecLocation.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted code from nwbRead and added to function io.spec.internal namespace to make it more accessible for testing

Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
function specLocation = readEmbeddedSpecLocation(fid, specLocAttributeName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the difference between readEmbeddedSpecLocaation and getEmbeddedSpecLocation? Can you add a comment here indicating the distinction?

Copy link
Collaborator Author

@ehennestad ehennestad Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getEmbeddedSpecLocation - Takes a filepath as an input.
readEmbeddedSpecLocation - Takes a fid as an input, useful if the file is already opened

arguments
fid (1,1) H5ML.id
specLocAttributeName (1,1) string = '.specloc'
end

specLocation = '';
try % Check .specloc
attributeId = H5A.open(fid, specLocAttributeName);
attributeCleanup = onCleanup(@(id) H5A.close(attributeId));
referenceRawData = H5A.read(attributeId);
specLocation = H5R.get_name(attributeId, 'H5R_OBJECT', referenceRawData);
catch ME
if ~strcmp(ME.identifier, 'MATLAB:imagesci:hdf5lib:libraryError')
rethrow(ME);

Check warning on line 15 in +io/+spec/+internal/readEmbeddedSpecLocation.m

View check run for this annotation

Codecov / codecov/patch

+io/+spec/+internal/readEmbeddedSpecLocation.m#L15

Added line #L15 was not covered by tests
end % don't error if the attribute doesn't exist.
end
end
16 changes: 16 additions & 0 deletions +io/+spec/getEmbeddedSpecLocation.m
Copy link
Collaborator Author

@ehennestad ehennestad Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted code from nwbRead and added to function io.spec namespace to make it more accessible for testing

Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
function specLocation = getEmbeddedSpecLocation(filename, options)
% getEmbeddedSpecLocation - Get location of embedded specs in NWB file
%
% Note: Returns an empty string if the spec location does not exist
%
% See also io.spec.internal.readEmbeddedSpecLocation

arguments
filename (1,1) string {matnwb.common.mustBeNwbFile}
options.SpecLocAttributeName (1,1) string = '.specloc'
end

fid = H5F.open(filename);
fileCleanup = onCleanup(@(id) H5F.close(fid) );
specLocation = io.spec.internal.readEmbeddedSpecLocation(fid, options.SpecLocAttributeName);
end
60 changes: 60 additions & 0 deletions +io/+spec/readEmbeddedSpecifications.m
Copy link
Collaborator Author

@ehennestad ehennestad Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted code from nwbRead and added to function io.spec namespace to make it more accessible for testing and improve modularity

Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
function specs = readEmbeddedSpecifications(filename, specLocation)
% readEmbeddedSpecifications - Read embedded specs from an NWB file
%
% specs = io.spec.readEmbeddedSpecifications(filename, specLocation) read
% embedded specs from the specLocation in an NWB file
%
% Inputs:
% filename (string) : Absolute path of an nwb file
% specLocation (string) : h5 path for the location of specs inside the NWB file
%
% Outputs
% specs cell: A cell array of structs with one element for each embedded
% specification. Each struct has two fields:
%
% - namespaceName (char) : Name of the namespace for a specification
% - namespaceText (char) : The namespace declaration for a specification
% - schemaMap (containers.Map): A set of schema specifications for the namespace

arguments
filename (1,1) string {matnwb.common.mustBeNwbFile}
specLocation (1,1) string
end

specInfo = h5info(filename, specLocation);
specs = deal( cell(size(specInfo.Groups)) );

fid = H5F.open(filename);
fileCleanup = onCleanup(@(id) H5F.close(fid) );

for iGroup = 1:length(specInfo.Groups)
location = specInfo.Groups(iGroup).Groups(1);

namespaceName = split(specInfo.Groups(iGroup).Name, '/');
namespaceName = namespaceName{end};

filenames = {location.Datasets.Name};
if ~any(strcmp('namespace', filenames))
warning('NWB:Read:GenerateSpec:CacheInvalid',...
'Couldn''t find a `namespace` in namespace `%s`. Skipping cache generation.',...
namespaceName);
return;

Check warning on line 41 in +io/+spec/readEmbeddedSpecifications.m

View check run for this annotation

Codecov / codecov/patch

+io/+spec/readEmbeddedSpecifications.m#L38-L41

Added lines #L38 - L41 were not covered by tests
end
sourceNames = {location.Datasets.Name};
fileLocation = strcat(location.Name, '/', sourceNames);
schemaMap = containers.Map;
for iFileLocation = 1:length(fileLocation)
did = H5D.open(fid, fileLocation{iFileLocation});
if strcmp('namespace', sourceNames{iFileLocation})
namespaceText = H5D.read(did);
else
schemaMap(sourceNames{iFileLocation}) = H5D.read(did);
end
H5D.close(did);
end

specs{iGroup}.namespaceName = namespaceName;
specs{iGroup}.namespaceText = namespaceText;
specs{iGroup}.schemaMap = schemaMap;
end
end
45 changes: 45 additions & 0 deletions +io/+spec/writeEmbeddedSpecifications.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted function from NwbFile/export to collect all code related to reading and writing embedded specifications in one place

Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
function writeEmbeddedSpecifications(fid, jsonSpecs)
specLocation = io.spec.internal.readEmbeddedSpecLocation(fid);

if isempty(specLocation)
specLocation = '/specifications';
io.writeGroup(fid, specLocation);
specView = types.untyped.ObjectView(specLocation);
io.writeAttribute(fid, '/.specloc', specView);
end

for iJson = 1:length(jsonSpecs)
JsonDatum = jsonSpecs(iJson);
schemaNamespaceLocation = strjoin({specLocation, JsonDatum.name}, '/');
namespaceExists = io.writeGroup(fid, schemaNamespaceLocation);
if namespaceExists
namespaceGroupId = H5G.open(fid, schemaNamespaceLocation);
names = getVersionNames(namespaceGroupId);
H5G.close(namespaceGroupId);
for iNames = 1:length(names)
H5L.delete(fid, [schemaNamespaceLocation '/' names{iNames}],...
'H5P_DEFAULT');
end
end
schemaLocation =...
strjoin({schemaNamespaceLocation, JsonDatum.version}, '/');
io.writeGroup(fid, schemaLocation);
Json = JsonDatum.json;
schemeNames = keys(Json);
for iScheme = 1:length(schemeNames)
name = schemeNames{iScheme};
path = [schemaLocation '/' name];
io.writeDataset(fid, path, Json(name));
end
end
end

function versionNames = getVersionNames(namespaceGroupId)
[~, ~, versionNames] = H5L.iterate(namespaceGroupId,...
'H5_INDEX_NAME', 'H5_ITER_NATIVE',...
0, @removeGroups, {});
function [status, versionNames] = removeGroups(~, name, versionNames)
versionNames{end+1} = name;
status = 0;
end
end
2 changes: 0 additions & 2 deletions +io/parseGroup.m
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved rehash statement to file.cloneNwbFileClass

Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,7 @@
parsed = NwbFile(kwargs{:});
else
file.cloneNwbFileClass(Type.name, Type.typename);
rehash();
parsed = io.createParsedType(info.Name, Type.typename, kwargs{:});

end

return;
Expand Down
Loading