-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Draft] Add transformation spec #63
Changes from all commits
4703d89
e72ee48
212b2dd
e5b23ee
df835f1
b3523c9
86d40fa
8d80dc4
821417f
74e3ca1
2abd87c
a4c2ac3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -205,6 +205,37 @@ Metadata {#metadata} | |
The various `.zattrs` files throughout the above array hierarchy may contain metadata | ||
keys as specified below for discovering certain types of data, especially images. | ||
|
||
"axes" metadata {#axes-md} | ||
-------------------------- | ||
|
||
Describes axes of a physical coordinate space. It is a dictionary, which MUST contain the fields: | ||
- "labels": list of strings that specify the name per dimension. The values MUST be unique. | ||
- "types": list of strings that specify the type per dimension. The values SHOULD be one of "space", "channel", "time". | ||
- "units": list of strings that specify the unit per dimension. | ||
|
||
The three lists MUST have the same length. | ||
If part of [[#multiscale-md]], the length MUST be equal to the array. | ||
|
||
|
||
"transformation" metadata {#trafo-md} | ||
------------------------------------- | ||
|
||
Describes transformations applied to an array. It is a dictionary, which MUST contain the field "type". | ||
The value of "type" MUST be one of the elements of the "type" column in the table below. | ||
Additional fields are defined by the column "fields". | ||
|
||
| type | fields | description | | ||
|- |- |- | | ||
| `identity` | | identity transformation, is the default transformation and is typically not explicitly defined | | ||
| `translation` | one of: `"translation":List[float]`, `"path":str` | translation vector, stored either as a list of floats (`"translation"`) or as binary data at a location in this container (`path`). The length of vector defines number of dimensions. | | ||
| `scale` | one of: `"scale":List[float]`, `"path":str` | scale vector, stored either as a list of floats (`scale`) or as binary data at a location in this container (`path`). The length of vector defines number of dimensions. | | ||
| `affine` | one of: `"affine":List[float]`, `"path:str"` | affine transformation matrix defined as list consisting of n sets of n + 1 scalar numbers, stored either as a list of floats (`affine`) or as binary data at a location in this container (`path`). n is number of dimensions | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be helpful to have an equation that illustrates the affine transformation and describes whether these lists are row-major, or column-major. Also, is it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we definitely need more examples and a clear definition here; I was a bit hesitant for the affines, because I copied this from https://github.com/saalfeldlab/n5-ij/wiki/Transformation-spec-proposal-for-NGFF, which (I think) is column major, but row major would be more appropriate for the more pythonic zarr spec. I will think about this, but I am currently leaning towards postponing the introduction of affines (I will elaborate below). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wish you great luck doing math with row major matrices in essentially all existing geometry frameworks ;). Joke aside, I had that same discussion with @d-v-b who is strongly pro row major vectors and we sort of agree that this makes no sense in the context of essentially all APIs that do geometry (unlike the pythonic zarr spec which doesn't) like OpenCV, OpenGL, ITK, ImgLib2, ... so please let's not do it here, it's a bad idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am not "pro" any specific kind of vectors :) I just want data like this to be consistent and explicit. Probably requiring a row- or column-major flag for vectors / matrices is the safest approach? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, let's please not confuse array indexing with geometry, it's bad enough that this creeps in with integer offsets and size vectors and other half-way in between stuff. It's ok to index elements in a matrix in y,x order, but I find it intolerably confusing to also re-sort the matrix elements. I also do not know a single textbook or geometry API that puts the last dimension first. This is certainly possible to deal with but will comsume pages of documentation and confuse generations of developers. I strongly suggest to stick with the x,y,z order for the elements of a matrix and vectors of transformations. Very opinionated here :). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the comments here :). I see the argument for sticking with the x,y,z convention re geometry...
Comment on lines
+229
to
+232
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i understand the explicitness of this. one thing to consider is whether just a dimensionality + 1 matched in the case where the data are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Given recent discussions (#57 (comment)) we will restrict the transformations available in the multiscale metadata (i.e. metadata for a single multiscale image pyramid) to
yes, the transformations are a list that are applied in order (this is also written clearly in the proposal)
check what exactly?
The field There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As the spec is currently written, I would expect a 6D affine for your
and or
(but this relates to the versions of the spec where the axis orders are fixed). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. more relevant though is @will-moore 's comment below on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the check was my misinterpretation that the transforms were independent. perhaps this sentence
from multiscale could be brought up to this section on transforms. that must apply to everything, right? thank you for the clarification of the other points. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fyi, this is still the case in the current spec proposal, just using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Absolutely! |
||
| `d_field` | `"path:str"` | deformation / displacement field storing one offset for each grid coordinate, `path` points to the array in this container that stores the field | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be more explicit and readable without having to refer to the spec, can we use In general, the spatial domain of a transform does not necessarily match the spatial domain of the image it transforms. Also, a transformation can be applied to point sets, etc. re: #14 . And, a displacement field is generally oversampled if it is sampled at the same resolution of the image it transforms, so there is a lot of wasted space when dealing with large images. So, we should associate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this and the next key should also consider whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Very good points. We should absolutely choose a more explicit name and specify how to deal with over-sampling.
for simplicity I would like to keep this in the same container in the first iteration; do you have some use-cases in mind where this would not work / require copying of too much data? See also general comment on trafo spec below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
since these are computed transforms, these are more likely to change as algorithms improve, and these transforms take up a lot of space. when you are versioning the data (even as a tree), this would thus lead to copies of the source data in many cases. since changes in software are more rapid that changes in source data, decoupling the transform to a different store/file/url would help. more generally i don't think the zarr world handles changes/versioning of datasets yet. there are some threads of discussion going on there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see and that makes sense. I am still a bit hesitant because external links could complicate the spec a lot. But maybe it would be a good idea to say SHOULD be an internal path but MAY also be an external path to accommodate development use cases. Then we would need some syntax for external paths though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why a reference to an external container complicates the spec? There are two main ways to resolve this such that container relative and path relative references are easy:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sorry, I was a bit imprecise here. I don't think that it would make the spec more complex, but it would have the added benefit of having all relevant data in the same container and not requiring any reference to external data (e.g. we also don't allow to reference to image data that is outside of the zarr container right now).
I would prefer this option. |
||
| `p_field` | `"path:str"` | position field storing one absolute position for each grid coordinate, `path` points to the array in this container that stores the field | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this is overlapping with functionality from the displacement field, can it be left out to simplify implementations? In the future, we will hopefully support composite / chaining of transformations. The displacement field can be easily chained, where a position field is not as amenable to chaining. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I copied this from https://github.com/saalfeldlab/n5-ij/wiki/Transformation-spec-proposal-for-NGFF. @bogovicj is there a use-case where we would need a positional field instead of a displacement field? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
In addition, the field "axisIndices" MAY be given to specify the subset of axes that the transformation is applied to, leaving other axes unchanged. If not given, the transformation is applied to all axes. The length of "axisIndices" MUST be equal to the dimensionality of the transformation. If "axisIndices" are not given, the dimensionality of the transformation MUST be equal to the number of dimensions of the array. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NB: Depending on the outcome of discussion at #54, this could be |
||
|
||
|
||
"multiscales" metadata {#multiscale-md} | ||
--------------------------------------- | ||
|
||
|
@@ -217,12 +248,13 @@ Each dictionary contained in the list MUST contain the field "datasets", which i | |
the arrays storing the individual resolution levels. | ||
Each dictionary in "datasets" MUST contain the field "path", whose value contains the path to the array for this resolution relative | ||
to the current zarr group. The "path"s MUST be ordered from largest (i.e. highest resolution) to smallest. | ||
All arrays MUST have the same number of dimensions and MUST NOT have more than 5 dimensions. | ||
Each dictionary MAY contain the field "transformations", which contains a list of [[#trafo-md]] that specify the transformation from data coordinates to the physical coordinates (as specified by "axes"). | ||
If given, the transformations MUST only be of type `identity`, `translation` or `scale`. This restriction ensures a simple mapping from data space to physical space and in particular that the voxel size can be directly read if a "scale" transformation is given. | ||
The transformations are applied sequentially and in order. If not given, the identity transformation is assumed. | ||
|
||
It MUST contain the field "axes", which is a list of dimension names of the axes. | ||
The values MUST be unique and one of `{"t", "c", "z", "y", "x"}`. | ||
The number of values MUST be the same as the number of dimensions of the arrays corresponding to this image. | ||
In addition, the "axes" values MUST be repeated in the field "_ARRAY_DIMENSIONS" of all scale groups | ||
(i.e. groups containing arrays with the multiscale data). | ||
It MUST contain the field "axes", see [[#axes-md]] and the length of the lists in "axes" must be equal to the number of dimensions in the array. | ||
The "labels" list must be repeated in the field "_ARRAY_DIMENSIONS" of all scale groups (i.e. groups containing arrays with the multiscale data). | ||
This ensures compatibility with the [xarray zarr encoding](http://xarray.pydata.org/en/stable/internals/zarr-encoding-spec.html#zarr-encoding). | ||
|
||
It SHOULD contain the field "name". | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid very troublesome ambiguities that result from missing information related to the direction of the physical space to physical space transformations, a SHOULD could be added here with
{ "from": "<string-identifier>", "to": "<string-identifier>" }
. Where, for example,<string-identifier>
is the name of an image or the name of an atlas space.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our idea in the original proposal was that all transformations are forward. It makes no sense to have a sequence of transformations where the from space of the second transformation is not the to space of the first, and if that's problematic, I do not see the point.
We also elaborated why the
inverse_of
transformation is a great idea: Images are usually transformed by iterating over target pixels, running their coordinates through the inverse of the transformation (which includes running it in inverse order through the inverses of the transformations of a sequence) and then interpolating in source space. This works without hassle for all transformations that are invertible (like e.g. most affines) but not for transformations that are not invertible (like displacement fields). For invertible displacement fields, the easiest solution is to provide both as in @bogovicj's atlas transformations. If only the inverse is required, because everything the transformation is to be used for is rendering transformed images, then it can be specified asinverse_of
a non-invertible transformation such as e.g. a deformation field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not explaining the
interleaved
field. We falsely assumed that this is self-explaining :). An interleavedd/p_field
stores the x,y,z target coordinates of the transformed x,y,z source coordinate contiguously, the non-interleavedd/p_field
stores first the x-coordinates, then the y-coordinates, then the z-coordinates. (I am explaining this by x,y,z example because opinions about dimension-indices are unresolved).