Change: Only embed specs/namespaces for types that are included in NWB file on export #615

ehennestad · 2024-11-04T19:17:20Z

Motivation

Ensure that only relevant namespaces/specifications are embedded in an NWB file. This issue suggests that such extra namespaces can cause errors when validating with NWBInspector.

How to test the behavior?

Export NWB file with types from a subset of available extension and check the file's `specification` group

Questions / todo

Are namespace names always specified in kebab-case? - Appears so
Does matnwb always format namespace names as snake_case? - Appears so
~~Should the specification group be completely cleared before rewriting it if exporting to an existing file?~~
Check superclasses of each type and included namespace of superclasses as well
Add unit test

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?

…port (Issue #607)

codecov · 2024-11-04T19:32:22Z

Codecov Report

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

Project coverage is 95.44%. Comparing base (30bf24d) to head (dc18c57).

Files with missing lines	Patch %	Lines
+io/+internal/+h5/resolveFileReference.m	87.50%	1 Missing ⚠️
+schemes/+utility/listNwbTypeHierarchy.m	87.50%	1 Missing ⚠️
NwbFile.m	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #615      +/-   ##
==========================================
+ Coverage   95.27%   95.44%   +0.17%     
==========================================
  Files         124      137      +13     
  Lines        5139     5265     +126     
==========================================
+ Hits         4896     5025     +129     
+ Misses        243      240       -3

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

Update docstring

Add utility function for listing the type hierarchy of an nwb type

…ataWithoutBorders/matnwb into add-nwb-install-extension

Add schedule event for workflow to update nwbInstallExtension

Remove local function

…ding-in-file

bendichter · 2025-01-22T20:40:14Z

+io/+internal/+h5/deleteGroup.m

@@ -0,0 +1,16 @@
+function deleteGroup(fileReference, groupLocation)
+% deleteGroup - Delete the specified group from an NWB file


I don't think it's a big deal that way this is used here, but note that deleting groups and datasets from an HDF5 file does not free up the space

bendichter · 2025-01-22T20:44:21Z

There are lots of new functions here that lack unit tests

…ding-in-file

…com/NeurodataWithoutBorders/matnwb into 607-fix-namespace-embedding-in-file

ehennestad · 2025-01-31T15:03:22Z

There are lots of new functions here that lack unit tests

@bendichter Added unit tests for those functions

bendichter · 2025-01-31T15:11:04Z

This seems quite complicated. Can you describe your general strategy here? I would think we could keep track of the namespaces used as we add neurodata objects to an NWB file and then only save the file with those namespaces. It looks here like you are doing a lot of manipulation on the HDF5 level. Can you help me understand this approach before I dig in?

ehennestad · 2025-02-01T20:47:21Z

This seems quite complicated. Can you describe your general strategy here? I would think we could keep track of the namespaces used as we add neurodata objects to an NWB file and then only save the file with those namespaces. It looks here like you are doing a lot of manipulation on the HDF5 level. Can you help me understand this approach before I dig in?

Please see the above comment/review.

Also see the docstring of io.spec.validateEmbeddedSpecifications for expanations of the HDF5 manipulations (i.e deleting groups)

Most of this PR is convenience functions. I added them to the io.internal.h5 namespace in anticipation of adding Zarr-functionality down the road

Added comments and a better test to test for warning with ID 'NWB:validators:MissingEmbeddedNamespace'

ehennestad · 2025-02-03T19:36:32Z

@bendichter I updated the test.

Only include namespaces for types that are included in NWB file on ex…

a458d62

…port (Issue #607)

ehennestad changed the title ~~Change: Only include namespaces for types that are included in NWB file on export~~ Change: Only embed specs/namespaces for types that are included in NWB file on export Nov 6, 2024

ehennestad added 27 commits November 23, 2024 13:21

Merge branch 'master' into 607-fix-namespace-embedding-in-file

13822a8

Add functionality for installing extensions

35150e9

Minor fixes

bd40894

Update comment

132aa67

Add comment + print message when extension has been installed

781f303

Update installExtension.m

eab998e

Fix changed variable name

be4be3d

Update matnwb_createNwbInstallExtension.m

650ceec

Update docstring

Create listNwbTypeHierarchy.m

8e18448

Add utility function for listing the type hierarchy of an nwb type

Add private method for embedding specifications to file on export

7c41ca6

Fix variable name

a467c46

Add workflow for updating nwbInstallExtension

ff95e98

Add option to save extension in custom location

bb3514b

Create InstallExtensionTest.m

184fa81

Update docstring

ddbe9dc

Merge branch 'master' into add-nwb-install-extension

44a6a20

Merge branch 'add-nwb-install-extension' of https://github.com/Neurod…

24c3899

…ataWithoutBorders/matnwb into add-nwb-install-extension

Change dispExtensionInfo to return info instead of displaying + add test

da00cea

Reorganize code into separate functions and add tests

40b7703

Merge branch 'master' into 607-fix-namespace-embedding-in-file

eeb2006

Merge branch 'master' into add-nwb-install-extension

e3b4906

Minor changes to improve test coverage

6faba21

add nwbInstallExtension to docs

a74a2d2

Update update_extension_list.yml

16877f9

Add schedule event for workflow to update nwbInstallExtension

Update downloadExtensionRepository.m

0bc735f

Remove local function

Update docstring for nwbInstallExtension

67680c2

Fix docstring indentation in nwbInstallExtension

b2e679a

ehennestad added 4 commits January 16, 2025 16:45

Create listEmbeddedSpecNamespaces.m

258b8dc

Update nwbExportTest.m

c694d10

Update test for spec/namespace embedding

8f0ec28

Merge branch 'add-nwb-install-extension' into 607-fix-namespace-embed…

7e9aac6

…ding-in-file

ehennestad marked this pull request as ready for review January 16, 2025 17:05

ehennestad added 2 commits January 22, 2025 11:07

Merge branch 'master' into 607-fix-namespace-embedding-in-file

09ec654

Update read_indexed_column.m

ab79e41

bendichter reviewed Jan 22, 2025

View reviewed changes

ehennestad added 10 commits January 22, 2025 23:26

Add disclaimer in deleteGroup function

7a32b7c

Update read_indexed_column.m

d97c83e

Fix broken test

863d319

add test-requirement

63c2194

Merge branch 'ehennestad-patch-fix-typo' into 607-fix-namespace-embed…

9acd164

…ding-in-file

Merge branch 'master' into 607-fix-namespace-embedding-in-file

b56c632

Fix: Ensure object is group before deleting

29bf432

Fix error id

680c4cf

Add unittests for functions in io.internal.h5 namespace

2a3c862

Merge branch '607-fix-namespace-embedding-in-file' of https://github.…

031949f

…com/NeurodataWithoutBorders/matnwb into 607-fix-namespace-embedding-in-file

ehennestad added 2 commits February 3, 2025 17:40

Merge branch 'master' into 607-fix-namespace-embedding-in-file

b14b9b8

Update nwbExportTest.m

4d7c5b7

Added comments and a better test to test for warning with ID 'NWB:validators:MissingEmbeddedNamespace'

ehennestad added 5 commits February 3, 2025 21:02

Merge branch 'main' into 607-fix-namespace-embedding-in-file

604e22c

Merge branch 'main' into 607-fix-namespace-embedding-in-file

0e5af2b

Fix failing tests

40f5de3

Merge branch 'main' into 607-fix-namespace-embedding-in-file

0d1128e

Merge branch 'main' into 607-fix-namespace-embedding-in-file

dc18c57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change: Only embed specs/namespaces for types that are included in NWB file on export #615

Change: Only embed specs/namespaces for types that are included in NWB file on export #615

ehennestad commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading

bendichter Jan 22, 2025

bendichter commented Jan 22, 2025

ehennestad commented Jan 31, 2025

bendichter commented Jan 31, 2025

ehennestad commented Feb 1, 2025 •

edited

Loading

ehennestad commented Feb 3, 2025

		@@ -0,0 +1,16 @@
		function deleteGroup(fileReference, groupLocation)
		% deleteGroup - Delete the specified group from an NWB file

Change: Only embed specs/namespaces for types that are included in NWB file on export #615

Are you sure you want to change the base?

Change: Only embed specs/namespaces for types that are included in NWB file on export #615

Conversation

ehennestad commented Nov 4, 2024 • edited Loading

Motivation

How to test the behavior?

Questions / todo

Checklist

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

bendichter Jan 22, 2025

Choose a reason for hiding this comment

bendichter commented Jan 22, 2025

ehennestad commented Jan 31, 2025

bendichter commented Jan 31, 2025

ehennestad commented Feb 1, 2025 • edited Loading

ehennestad commented Feb 3, 2025

ehennestad commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading

ehennestad commented Feb 1, 2025 •

edited

Loading