Refactor: Add arguments block to main MatNWB api functions #619
Conversation
- Move some functions io.spec namespace - Add validation functions in matnwb.common namespace
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #619 +/- ##
==========================================
+ Coverage 90.85% 95.16% +4.30%
==========================================
Files 107 113 +6
Lines 4781 4734 -47
==========================================
+ Hits 4344 4505 +161
+ Misses 437 229 -208 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
- Simplify logic in fillExport
continue is unecessary here
Add arguments block
Removes lines that can not be reached
Simplify
There was a problem hiding this comment.
Extracted function from NwbFile/export to collect all code related to reading and writing embedded specifications in one place
There was a problem hiding this comment.
Moved rehash statement to file.cloneNwbFileClass
There was a problem hiding this comment.
Use onCleanup to close space and object instead of doing it "conditionally"
There was a problem hiding this comment.
Add utility function for finding the latest available nwb schema version
There was a problem hiding this comment.
Add validator function to assert that a path is an NWB filepath
There was a problem hiding this comment.
Add validator function to assert that a versionNumber value is a properly formatted version number string and that the version number is available among the available nwb schemas
There was a problem hiding this comment.
- Specify default values in property block instead of constructor
- Use assert instead of if to improve coverage
There was a problem hiding this comment.
- Also allow
schemaSourcebeing a string - use
filereadinstead offopen,fwrite,fclosesequence
There was a problem hiding this comment.
- Add named arguments and arguments block to simplify argument parsing
- Negate
isempty(fileList)block to remove potentially unreachable code
There was a problem hiding this comment.
Add unit tests to increase coverage for nwbRead, nwbExport
There was a problem hiding this comment.
Add unites to test the file.cloneNwbFileClass function
| @@ -131,6 +142,9 @@ function testExternalFilters(testCase) | |||
| import types.untyped.datapipe.properties.DynamicFilter; | |||
| import types.untyped.datapipe.properties.Shuffle; | |||
|
|
|||
| % TODO: Why is Filter.LZ4 not part of the exported Pipe, i.e when the | |||
There was a problem hiding this comment.
@bendichter @lawrence-mbf I need help to understand if this is a bug
There was a problem hiding this comment.
Result of updating class generator to fix word capitalisation
There was a problem hiding this comment.
Added arguments block
There was a problem hiding this comment.
Move default output to beginning as it was potentially unreachable for unit tests
There was a problem hiding this comment.
Removed unused method
There was a problem hiding this comment.
Added named arguments and argument block
There was a problem hiding this comment.
Fixed to work for older schema versions
There was a problem hiding this comment.
- Add arguments block to constructor and export method
- Added mode for overwriting file on export
- Use assertion instead of if block with error on export to improve coverage
- removed
embedSpecificationsmethod. Was moved toio.spec.writeEmbeddedSpecifications
There was a problem hiding this comment.
moved file into resources folder
There was a problem hiding this comment.
- Add named arguments and arguments block
- Pass
namespaceFilescell array to generateExtension instead of potentially calling it twice
There was a problem hiding this comment.
- Add named arguments and arguments block
- Added local validator function
mustBeYamlFleto check that namespace spec files exist - simplified code in loop for generating class files
There was a problem hiding this comment.
- Add optional output argument. Can be used by cleanup operations to regenerate cleared namespaces
- Add input for specifying root folder where classes to be cleared exist
- Added option for also clearing the namespace cache
There was a problem hiding this comment.
- Rename input arguments to be more descriptive
- Added argument blocks for simpler input handling / validation
There was a problem hiding this comment.
Moved to resources folder
Changed to be up-to-date
| @@ -0,0 +1,18 @@ | |||
| function specLocation = readEmbeddedSpecLocation(fid, specLocAttributeName) | |||
There was a problem hiding this comment.
what's the difference between readEmbeddedSpecLocaation and getEmbeddedSpecLocation? Can you add a comment here indicating the distinction?
There was a problem hiding this comment.
getEmbeddedSpecLocation - Takes a filepath as an input.
readEmbeddedSpecLocation - Takes a fid as an input, useful if the file is already opened
| function closeSpaceAndObject(spaceId, objectId) | ||
| H5S.close(spaceId); | ||
| H5O.close(objectId); | ||
| end |
There was a problem hiding this comment.
This feels like it could have some side effects. Are you able to separate this change into a different PR?
There was a problem hiding this comment.
I would argue that it is more robust than the current code.
When h5CleanupObj is cleared from the workspace, i.e when the function exits, either on returning or on errors, it will run the callback and close the space and objects.
At the moment if the code fails on line 16-18 or 26, these would not be closed, but with the cleanup callback they will
Fix #606
Motivation
How to test the behavior?
Checklist
fix #XXwhereXXis the issue number?