Multiscales metadata API: only support datasets as lists of dictionaries #165
Conversation
Codecov Report
@@ Coverage Diff @@
## master #165 +/- ##
==========================================
+ Coverage 83.53% 83.93% +0.40%
==========================================
Files 12 12
Lines 1354 1357 +3
==========================================
+ Hits 1131 1139 +8
+ Misses 223 218 -5
Continue to review full report at Codecov.
|
| if isinstance(dataset, str): | ||
| validated_datasets.append({"path": dataset}) | ||
| elif isinstance(dataset, dict): | ||
| if isinstance(dataset, dict): |
There was a problem hiding this comment.
This might have been simpler as assert isinstance(dataset, dict)
There was a problem hiding this comment.
Now I'm pondering if there's some generic assertion or validation module that one could use to skip for loops where something isn't valid without just throwing a ValueError.
There was a problem hiding this comment.
No strong opinion on using assert vs conditional and a custom error like ValueError. If we collectively agree on the former version for now, I can quickly go over the codebase after this PR.
My mid-term hope is to replace this internal validation code by a generic validation framework based on schemas/constraints. I expect this should also bring typical features like being able to collect all validation errors like the Validator.iter_errors() API of the jsonschema module.
There was a problem hiding this comment.
Exactly. No strong opinion on assert isinstance especially if we'll be reviewing soon anyway.
Follow-up of the discussion in #162, this PR primarily the
write_multiscales_metadataAPI introduced in #149 to only work ondatasetspassed as list of dictionaries. The writer unit tests are all adjusted for different version of the OME-NGFF format.Additional unit tests are also added testing various combinations of valid and invalid
coordinateTransformations. The transformations validation method is updated to add a few extra checks on the presence of mandatory keys.