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.

Sign up for GitHub

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

Ensure core and hdmf-common specifications are embedded in NWBFile by default #674

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Ensure core and hdmf-common specifications are embedded in NWBFile by default #674

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1e9c961
Fix issue #673
ehennestad Mar 1, 2025
1fce887
Update nwbExportTest.m
ehennestad Mar 1, 2025
e307dff
Modify spec embedding so that relevant tests are passing
ehennestad Mar 1, 2025
c98b898
Update NwbFile.m
ehennestad Mar 5, 2025
8f44c79
Improve warning message if namespace specifications are not embedded …
ehennestad Mar 5, 2025
b7632e3
Replace Clustering with SpatialSeries in PyNWBIOTest
ehennestad Mar 5, 2025
429d0be
Merge branch 'remove-clustering-from-testsuite' into 673-embed-specs-…
ehennestad Mar 5, 2025
d761cd6
Merge branch 'main' into 673-embed-specs-in-empty-file
ehennestad Mar 6, 2025
abf26da
Fix bug causing a test to fail
ehennestad Mar 6, 2025
d8af421
Merge branch '673-embed-specs-in-empty-file' of https://github.com/Ne…
ehennestad Mar 6, 2025
274745e
Add check on path name before deleting folder in nwbClearGenerated
ehennestad Mar 6, 2025
3fee57e
Need to generateCore with the correct version if cached namespaces ar…
ehennestad Mar 6, 2025
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
8 changes: 7 additions & 1 deletion +io/+spec/validateEmbeddedSpecifications.m
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Improve warning message

Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,13 @@ function checkMissingNamespaces(expectedNamespaceNames, embeddedNamespaceNames)
missingNamespaces = setdiff(expectedNamespaceNames, embeddedNamespaceNames);
if ~isempty(missingNamespaces)
missingNamespacesStr = strjoin(" " + string(missingNamespaces), newline);
warning('NWB:validators:MissingEmbeddedNamespace', 'Namespace is missing:\n%s', missingNamespacesStr)
warning('NWB:validators:MissingEmbeddedNamespace', ...
['The following namespace specifications are not embedded in ' ...
'the file:\n%s\n' ...
'To facilitate reading and validating the file across systems, it is ' ...
'recommended to embed the specifications for these namespaces. ' ...
'Please generate the missing extensions (using generateCore or ' ...
'generateExtension) and then re-export the file.'], missingNamespacesStr)
end
end

Expand Down
2 changes: 1 addition & 1 deletion +tests/+unit/dynamicTableTest.m
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix unrelated bug in test suite

Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function testToTableForTableImportedFromFile(testCase)

nwbExport(nwb, fileName)

nwbIn = nwbRead(fileName);
nwbIn = nwbRead(fileName, "ignorecache");

T = nwbIn.acquisition.get('DynamicTable').toTable();
testCase.verifyClass(T, 'table')
Expand Down
4 changes: 2 additions & 2 deletions +tests/+unit/nwbExportTest.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ function testEmbeddedSpecs(testCase)
nwbFilePath = testCase.getRandomFilename();
nwbExport(nwb, nwbFilePath);

% Verify that no namespaces were embedded in file
% Verify that core and hdmf-common were embedded in "empty" file
embeddedNamespaces = io.spec.listEmbeddedSpecNamespaces(nwbFilePath);
testCase.verifyEmpty(embeddedNamespaces)
testCase.verifyEqual(sort(embeddedNamespaces), {'core', 'hdmf-common'})

ts = tests.factory.TimeSeriesWithTimestamps();
nwb.acquisition.set('test', ts);
Expand Down
21 changes: 19 additions & 2 deletions NwbFile.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@
arguments
obj (1,1) NwbFile
options.IncludeParentTypes (1,1) logical = false
options.IncludeNwbFile (1,1) logical = false

Check warning on line 126 in NwbFile.m

View check run for this annotation

Codecov / codecov/patch

NwbFile.m#L126 

Added line #L126 was not covered by tests
end

objectMap = searchProperties(containers.Map, obj, '', '');
Expand All @@ -136,6 +137,14 @@

nwbTypeNames = objectClassNames(keep & ~ignore);

if options.IncludeNwbFile
% Include class name for NWBFile superclass
allSuperclasses = string(superclasses(obj));
nwbTypeNames = [...
allSuperclasses(endsWith(allSuperclasses, 'NWBFile')), ...
nwbTypeNames];
end

if options.IncludeParentTypes
includedNwbTypesWithParents = string.empty;
for i = 1:numel(nwbTypeNames)
Expand All @@ -152,12 +161,20 @@
function embedSpecifications(obj, output_file_id)
jsonSpecs = schemes.exportJson();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This works, but ideally namespaces would get parsed and saved in the mat-file cache in the schemes.exportJson() function if the are missing.

This would require a bit more refactoring though, and not sure if that is worth it @bendichter ?


if isempty(jsonSpecs)
% Call generateCore to create cached namespaces
generateCore(obj.nwb_version)
jsonSpecs = schemes.exportJson();

Check warning on line 167 in NwbFile.m

View check run for this annotation

Codecov / codecov/patch

NwbFile.m#L166-L167 

Added lines #L166 - L167 were not covered by tests
end

% Resolve the name of all types and parent types that are
% included in this file. This will be used to filter the specs
% to embed, so that only specs with used neurodata types are
% embedded.
includedNeurodataTypes = obj.listNwbTypes("IncludeParentTypes", true);

includedNeurodataTypes = obj.listNwbTypes(...
"IncludeParentTypes", true, ...
"IncludeNwbFile", true);

% Get the namespace names
namespaceNames = getNamespacesForDataTypes(includedNeurodataTypes);

Expand Down
4 changes: 3 additions & 1 deletion nwbClearGenerated.m
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix unrelated issue. Make sure the path is actually a folder before calling rmdir

Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
moduleNames = setdiff({listing.name}, {'+untyped', '+util', '.', '..'});
generatedPaths = fullfile(typesPath, moduleNames);
for i=1:length(generatedPaths)
rmdir(generatedPaths{i}, 's');
if isfolder(generatedPaths{i})
rmdir(generatedPaths{i}, 's');
end
end

if options.ClearCache
Expand Down