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

Jump to bottom

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

wants to merge 12 commits into from

Conversation

ehennestad
Copy link
Collaborator

@ehennestad ehennestad commented Mar 1, 2025 •
edited
Loading

Fix #673

Motivation

Ensure core and hdmf-common specifications are embedded by default.

How to test the behavior?

nwb = tests.factory.NWBFile();
nwbExport(nwb, 'test.nwb');

% Verify that core and hdmf-common were embedded in "empty" file
embeddedNamespaces = io.spec.listEmbeddedSpecNamespaces('test.nwb');
assert( isequal( sort(embeddedNamespaces), {'core', 'hdmf-common'}) )

Checklist

  • Have you ensured the PR description clearly describes the problem and solutions?
  • Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
  • If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

@ehennestad ehennestad requested a review from bendichter March 1, 2025 08:30
@ehennestad ehennestad mentioned this pull request Mar 1, 2025
Closed
3 tasks
@codecov Codecov
Copy link

codecov bot commented Mar 6, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.21053% with 3 lines in your changes missing coverage. Please review.

Project coverage is 95.28%. Comparing base (7d73c2b) to head (3fee57e).

Files with missing lines Patch % Lines
NwbFile.m 75.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #674      +/-   ##
==========================================
- Coverage   95.33%   95.28%   -0.05%     
==========================================
  Files         139      139              
  Lines        5311     5328      +17     
==========================================
+ Hits         5063     5077      +14     
- Misses        248      251       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ehennestad
ehennestad commented Mar 6, 2025
View reviewed changes
+io/+spec/validateEmbeddedSpecifications.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.

Improve warning message

ehennestad
ehennestad commented Mar 6, 2025
View reviewed changes
+tests/+unit/dynamicTableTest.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.

Fix unrelated bug in test suite

ehennestad
ehennestad commented Mar 6, 2025
View reviewed changes
nwbClearGenerated.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.

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

ehennestad
ehennestad commented Mar 6, 2025
View reviewed changes
NwbFile.m
@@ -152,12 +161,20 @@ function export(obj, filename, mode)
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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bendichter bendichter Awaiting requested review from bendichter

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Schema are not cached in NWB file
1 participant
@ehennestad