Coordinate transforms json-schema (request for feedback) #149
Conversation
# Conflicts: # latest/index.bs
* define pixel center coordinate system * add input/output_axes * add transform inverse flag * add affine transform type
* flesh out array space * define array indexing * add more transformation types * start transformation details section and examples * update example
# Conflicts: # latest/index.bs
* use "input" and "output" rather than '*Space' and '*Axes'
* reorder details * clean up table * add rotation * details for sequence * describe inverses * wrap examples
* rephrase matrix storage
* change to "coordinates", removing "Field" * change to "displacements", removing "Field"
* add details for transformation types * (identity, inverseOf, bijection) * describe inputAxes and outputAxes * add new examples
…amples * some clean up
* add mapIndex, mapAXis * add examples * affine stored as flat array only
* sequence does not have by-dimension behavior
* flesh out some examples
Note that the test currently does not pass because displacements have not been defined yet.
The tests mark invalid pass because we would need dynamic validation to check whether the axes were present in the coordinate spaces.
Automated Review URLs |
| "type": "scale", | ||
| "scale": [0.1, 1.0, 0.5, 0.5, 0.5], | ||
| "input" : "/0", | ||
| "output" : "xampleCoordinateSystem" |
There was a problem hiding this comment.
| @@ -0,0 +1,64 @@ | |||
| { | |||
| "coordinateSystems" : [ | |||
| { "name " : "in", "axes" : [ {"name" : "0", "name" : "1", "name": "2", "name": "3", "name": "4" }] }, | |||
There was a problem hiding this comment.
The axes list has a single dict with multiple "name" keys? Is is supposed to be [{"name" : "0"}, {"name" : "1"}..] ?
There was a problem hiding this comment.
| { "name " : "in", "axes" : [ {"name" : "0", "name" : "1", "name": "2", "name": "3", "name": "4" }] }, | |
| { "name ": "in", "axes": [{"name" : "0"}, {"name" : "1"}, {"name": "2"}, {"name": "3"}, {"name": "4"}] }, |
@bogovicj, should changes to the examples happen here or in your PR?
I haven't modified these from your branch, but also could add tests for the subspace directory.
There was a problem hiding this comment.
| { | ||
| "coordinateSystems" : [ | ||
| { "name " : "in", "axes" : [ {"name" : "0", "name" : "1", "name": "2", "name": "3", "name": "4" }] }, | ||
| { "name " : "out", "axes" : [ {"name" : "x", "name" : "y", "name" : "z" }] } |
| "coordinateTransformations" : [ | ||
| { | ||
| "type" : "sequence", | ||
| "name" : "5D-to-3D", |
There was a problem hiding this comment.
Does this combination of mapIndex and scale really go from 5D data to 3D? And same for 5D-to-3D-not-contiguous examples below?
| "time", | ||
| "channel", | ||
| "coordinate", | ||
| "displacement" |
There was a problem hiding this comment.
The spec proposal says It SHOULD be one of "array", "space", "time", "channel", "coordinate", or "displacement" but MAY take other values for custom axis types that are not part of this specification yet but this schema wouldn't allow any other custom types.
There was a problem hiding this comment.
How should schemas for this repository represent "MAY"?
Is this what's supposed to be in the strict_* schemas?
There was a problem hiding this comment.
The strict schemas are designed to give warnings that encourage more complete metadata (e.g. multiscales SHOULD have a "name"). If someone has used some axes "type": "my_custom_type" there is probably a good reason (and it maybe shouldn't fail a strict schema validation). However the spec does say It SHOULD be one of "array", "space", "time"... and the strict schemas are used for SHOULD rules, so that is probably the right place to add that (rather than not include it at all).
Hopefully it's not too tricky to move it there?
| }] | ||
| } | ||
| ], | ||
| "coordinateTransformations": [{ |
There was a problem hiding this comment.
Is the top-level multiscales coordinateTransformations no-longer valid? If it is, then this is a useful example. If not then the spec wording needs to be changed.
There was a problem hiding this comment.
It will still be valid - I'll edit this example showing how.
| "datasets": [ | ||
| { | ||
| "path": "0" | ||
| // the transformation of other arrays are defined relative to this, the highest resolution, array |
There was a problem hiding this comment.
The spec still says:
"Each dictionary in "datasets" MUST contain the field "coordinateTransformations", which contains a list of transformations that map the data coordinates to the physical coordinates (as specified by "axes") for this resolution level.
The transformations are defined according to [[#trafo-md]]. The transformation MUST only be of type translation or scale.
They MUST contain exactly one scale transformation that specifies the pixel size in physical units or time duration. If scaling information is not available or applicable for one of the axes, the value MUST express the scaling factor between the current resolution and the first resolution for the given axis, defaulting to 1.0 if there is no downsampling along the axis."
There was a problem hiding this comment.
@bogovicj, assuming I should go with the spec over the example here?
Out of interest, why does the pixel -> physical need to be replicated here?
There was a problem hiding this comment.
I think the spec just needs updating or clarifying at that point. Maybe now it's
"Each dictionary in "datasets" except the first MUST contain the field "coordinateTransformations"...
# Conflicts: # latest/index.bs
Now that main has been merged into coord-transforms
|
@ivirshup, after a long break and silence, I looked at this PR, and it's been a huge help in updating the schema! Soon (today?) I'm going to merge this into my branch, and your commits will be merged when my PR is merged. I'll also try to address some of the issues caught bu @will-moore, thanks for catching them! |
json-schemafor coordinate transforms proposal #138.This branch is based off the spec PR. The spec was only modified to move inline examples to the
examplesdirectory. All other changes should be to files under:This isn't 100% done, but I would appreciate feedback on the implementation and structure of the schemas.
The plan
@bogovicj and I would like to get feedback on the schema here. At a later point we would like to merge it with the coordinate transform spec.
What's left to do:
"description"fields. These should be added, but likely should just be copied from the spec.Questions from me
Current state of tests
Since the CI isn't currently running, I figured I'd show the state of the tests here:
pytest output