Transformations0.6#80
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
It would be good to test this with @jo-mueller's test samples listed at https://forum.image.sc/t/ngff-weekly-dev-update-thread/110810/98. |
|
German-BioImaging/incubator#72 Sample images to test against |
I will mirror these in the image.sc dev thread |
davehorsfall
left a comment
There was a problem hiding this comment.
This is good. Left some comments for discussion.
| const parsedData = parse(node.attrs); | ||
| if (parsedData.version === "v06") { | ||
| if (parsedData.type === "SceneSchema") { | ||
| //Temporary assertion until parsing layer implemented |
There was a problem hiding this comment.
If it doesn't already exist, can we create an Issue in the Backlog to follow up on this. Thanks.
There was a problem hiding this comment.
Added note/sub-issue to existing issue #64
| coordinateSystems?: CoordinateSystem[]; | ||
| } | ||
|
|
||
| //Once parsing and transforming is set up, it should be possible to significantly simplify these types |
There was a problem hiding this comment.
Please also point here from the Issue that is setup to track full implementation of the transformations from the ZOD schema parsing.
| const parser = validParsers[validParsers.length - 1]; | ||
|
|
||
| return { | ||
| data: parser.schema.parse(data), |
There was a problem hiding this comment.
What happens when the schema can't parse the data? How is this handled?
There was a problem hiding this comment.
Schema parsing uses the safeParse method which prevents errors from being raised if the metadata does not match the given schema.
| ); | ||
| console.log("Applying scene transformations to image: ", config.source); | ||
| const sceneModelMatrix = coordinateTransformationsToMatrix(transformations, [ | ||
| { type: "channel", name: "c" }, |
There was a problem hiding this comment.
This appears to be passing a hard-coded 4-element [c, z, y, x] axes array to coordinateTransformationsToMatrix regardless of the scene's actual coordinate systems. Is that intended, and long term?
coordinateTransformationsToMatrix also seems to throw and error if the shape of the axes and the transformations don't match.
There was a problem hiding this comment.
Changed to dynamically select the first coordinate system axes in the scene metadata.
Left a TODO for future work to allow the user to change between coordinate systems.
Linked TODO to sub-issue in existing issue #69
| const labels = await resolveOmeLabelsFromMultiscales(grp); | ||
| const modelMatrix = coordinateTransformationsToMatrix( | ||
| getOrderedTransformations(attrs.multiscales, selectedCoordinateSystem), | ||
| coordinateSystems[0].axes, |
There was a problem hiding this comment.
Should this perhaps be selectedCoordinateSystem.axes, or what is the selectedCoordinateSystem intended to be used for?
There was a problem hiding this comment.
TL;DR
It is the coordinateSystem name that is important for this function, not the axes. I have removed the nested function calls for clarity.
Explanation
The scene metadata defines which output coordinate systems should be used for each multiscale image in the scene (we can imagine that each multiscale image may have multiple coordinate systems and multiple transformations associated with each coordinate system). So, to create the multiscale image, the loadScene function takes the name of the selectedCoordinateSystem from the scene metadata. This is then used to filter which transformations are actually applied to the image.
This may be more clear from the example data:
Scene:
Scene.coordinateTransformations[0].input defines:
input: { "path": "4995115_full.zarr" , "name": "physical" } ,
i.e. the path to the image and the coordinate system of the image to use.
Multiscale image
Defines:
coordinateSystems: [
{
"name": "physical" ,
axes: [
{ ... } , // 2 items
{ ... } , // 3 items
{ ... } , // 3 items
{ ... } // 3 items
]
}
]
and transformation:
transformations: [
{ ... } , // 2 items
{ ... } // 2 items
] ,
"input": "s0" ,
"output": "physical"
}
]
| mat = coordinateTransformationsToMatrix(transform.transformations, axes, mat); | ||
| } | ||
| if (transform.type === "affine") { | ||
| console.log("Affine transformation detected"); |
There was a problem hiding this comment.
Perhaps also add an Issue to track support for this as it develops, and as test datasets start to be generated/shared.
There was a problem hiding this comment.
Uncommented this as it actually works - it doesn't have any impact on the test image because the affine transformation defined is the identity. I think this was commented out while debugging at some point.
| //No type information for SceneSchema | ||
| scene: Ome.Scene, | ||
| ): Promise<SourceData[]> { | ||
| console.log("Loading scene: ", config.source); |
There was a problem hiding this comment.
Currently there are lots of informational logs printed to the console (even in production?)
It would be good to start wrapping this into the app level logger (and error handler) we've been discussing, so it is easier to control what is printed, and where.
There was a problem hiding this comment.
Agreed to add these once logging PR has been merged #95
|
@davehorsfall addressed comments and workflows passing. |
|
https://ome.github.io/ome-ngff-validator/?source=https://livingobjects.ebi.ac.uk/idr/zarr/test-data/v0.6.dev4/idr0050/4995115_output_to_ms.zarr is a more up-to-date scene example, although slightly more complex than the v0.6.dev3 one above. The dev3 one is failing validation because it's using latest schemas which have been updated for dev4. |
Description
Implements scene specification and 0.6 multiscales coordinateSystems.
Adds test images including scene. URL can be found at fixtures/scene.yaml e.g.
https://uk1s3.embassy.ebi.ac.uk/idr/zarr/test-data/v0.6.dev3/idr0050/4995115_scene.zarr
NOTE
Due to image metadata mis-specification, the image WILL NOT align correctly while conforming to the specification rules. However, if the additional 'rotated' transformations are included the image DOES align correctly (i.e. the rotation transformation logic is correctly implemented). This is because the scene-level transformations specified here
https://will-moore.github.io/ome-ngff-validator/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/test-data/v0.6.dev3/idr0050/4995115_scene.zarr
for each image have input coordinate system 'physical', meaning we should only apply the coordinate transformations with coordinate system 'physical' from these images.