-
Notifications
You must be signed in to change notification settings - Fork 42
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
Finalize axes & initial transformation #85
Conversation
latest/index.bs
Outdated
They MUST contain at most one `scale` transformation that specifies the pixel size in physical units or time duration. | ||
It also MUST contain at most one `translation` that specifies the offset from the origin in physical units. | ||
The length of the `scale` and/or `translation` array MUST be the same as the length of "axes". | ||
If both `scale` and `translation` are given `translation` must be listed after `scale` to ensure that it is given in physical coordinates. If "coordinateTransformations" is not given, the identity transformation is assumed. |
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.
@will-moore we actually restrict the order of scale and translation here to avoid the ambiguity discussed earlier. I suggest we leave this as is for v0.4. (cc @bogovicj, for now we probably don't need a longer explanation on transformation order; but I am sure this will become relevant for the advanced transformation proposal ;))
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.
Sounds good,
As you say I'll still write the explanation on order that I was planning on, but indeed - now you don't need it.
Thanks @constantinpape !
latest/index.bs
Outdated
} | ||
], | ||
"transformations": [{"type": "scale", "scale": [0.1], "axisIndices": [0]], # the time unit (0.1 milliseconds), which is the same for each scale level | ||
"transformations": [{"type": "scale", "scale": [0.1, 1.0, 1.0, 1.0, 1.0]], # the time unit (0.1 milliseconds), which is the same for each scale level |
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.
If we decide to drop the 'global' transformation we need to remove it here as well and move the 0.1 into the per scale transforms.
@@ -263,16 +261,17 @@ Each dictionary in "datasets" MUST contain the field "path", whose value contain | |||
to the current zarr group. The "path"s MUST be ordered from largest (i.e. highest resolution) to smallest. | |||
|
|||
Each "datasets" dictionary MUST have the same number of dimensions and MUST NOT have more than 5 dimensions. The number of dimensions and order MUST correspond to number and order of "axes". | |||
Each dictionary MAY contain the field "transformations", 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]]. In addition, the transformation types MUST only be `identity`, `translation` or `scale`. | |||
They MUST contain at most one `scale` transformation per axis that specifies the pixel size in physical units. |
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.
The "per axis" should be kept if we keep the 'global' transformation.
While we can still rename things, can we consider renaming |
latest/index.bs
Outdated
The requirements (only `scale` and `translation`, restrictions on order) are in place to provide a simple mapping from data coordinates to physical coordinates while being compatible with the general transformation spec. | ||
|
||
Each "multiscales" dictionary MAY contain the field "coordinateTransformations", describing transformations that are applied to each resolution level. | ||
The transformations MUST follow the same rules about allowed types, order, etc. as in "datasets:coordinateTransformations". |
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.
Now that we have resolved the "datasets:coordinateTransformations", let's return to whether we want to keep the "multiscales:coordinateTransformations", i.e. a transformation that is applied the same way to all scale levels after the individual transforms per scale level. For more context: the motivation for this feature is to refactor a transformation that is applied to all scale levels, e.g. scaling the time interval to be 0.1 seconds:
"axes": [{"name": "t", "type": "time", "unit": "seconds"}, {"name": "y", "type": "space", "unit": "meter"}, {"name": "x", "type": "time", "unit": "meter"}],
# version with transformation only in datasets:
"datasets": [
{"coordinateTransformations": [{"type": "scale", "scale": [0.1, 0.2, 0.2]}]}, # scale-level 0, phyiscal size is 20 cm
{"coordinateTransformations": [{"type": "scale", "scale": [0.1, 0.4, 0.4]}]} # scale-level 1, physical size is 40 cm, time scale is the same
]
# version with additional transformation in multiscales
"datasets": [
{"coordinateTransformations": [{"type": "scale", "scale": [1.0, 0.2, 0.2]}]}, # scale-level 0, phyiscal size is 20 cm
{"coordinateTransformations": [{"type": "scale", "scale": [1.0, 0.4, 0.4]}]} # scale-level 1, physical size is 40 cm
]
"coordinateTransformations": [{"type": "scale", "scale": [0.1, 1.0, 1.0]}] # apply the timescale for both resolutions.
For our current transformations it is trivial to express the scale (or translation) in "multiscales:coordinateTransformations" without using "datasets:coordinateTransformations". However, for advanced transformations this is different: take the example of a non-uniform time axis. We could express this with a transformation that has a value for each discrete point along one axis, e.g. 100 values if we have 100 time points. In this case it would be much better to specify this once in "multiscales" and not several times in "datasets".
Given that we don't have this use-cases yet, I would vote to remove "multiscales:coordinateTransformations" from the current version to keep it simple. We can then introduce it (or a similar approach) once it becomes necessary in the next version. But I don't have a very strong opinion on this and would be ok with keeping it if a majority finds it useful already.
cc @bogovicj @will-moore (and comments are welcome from everyone of course)
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.
My understanding is that so far any transformations (including advanced transformations) can always be expressed using only dataset:coordinateTransformations
and defining a transformation property at a higher level is primarily for optimization/efficiency purposes.
Within the scope of 0.4, my personal feeling is that the second form proposed above does not offer significant advantages while increasing the complexity of the metadata. For me, that's an incentive to leave this it out of the specification for now and introduce it as part of the next body of work when there are clear use cases.
Also, I assume you are talking about introducing the coordinateTransfomations
level at the individual multiscale
level i.e. within an element of the multiscales
array. One could also imagine defining it at the multiscales
level i.e. a transformation that applies to all the multiscales lists (also true for axes
).
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.
Either in 0.4 or at a later time we will want the global coordinate transformation for the use case of registration. With coordinate transformations that are the output of registration methods, they typically apply to all scales in the same way. While technically possible to duplicate these over the scales, when we support displacement field transformations this will result in unnecessarily large duplicated storage unless the path
feature is used.
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.
👍 thanks for the input @thewtex. Re-reading myself, I think my comment was hyperfocusing on the use case described in #85 (comment) and I still think a top-level scale
element brings little value, primarily because scale
is proposed to be mandatory at every dataset
level.
The registration use case you are bringing up is a good one. Would it be useful to construct such an example where each dataset contains a scale
and the multiscale object contains a translation
? /cc @constantinpape
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.
Neuroglancer internally uses a very similar multiscale volume representation to what is proposed here. It specifies the following:
- For the top-level multiscale volume, there is a coordinate space that indicates:
- Name of each dimension
- Unit and scale for each dimension, e.g. "4 nm" or "0.5 s" or "" (for unitless dimensions like channel dimensions), that indicates the "native resolution" of the dataset. For example if the unit is "4 nm", then changing the coordinate by 1 corresponds to a shift of 4 nm in physical space.
- Optional lower and upper bounds for each dimension.
- Then for each scale there is an affine transform to transform from the voxel space of the scale to the native resolution indicated at the top level. Typically the affine transform for the first scale is translation only, and the affine transform for subsequent levels are scale-and-translation-only, where the scale factors are the downsampling factors relative to the first scale.
While it breaks down once you introduce rotation or non-linear warping, the concept of a "native resolution" is very useful in practice when dealing with only translation and scale, so it may be valuable to preserve that in the ome-zarr spec.
While just supporting both per-multiscale and per-scale transformation may be somewhat useful to allow a more concise representation, I think it could be even more useful to specify the purpose of the separate transformations. For example, if we wanted to follow what Neuroglancer does internally, we could say that the per-scale transformations should transform to the "native" resolution of the dataset, while the top-level transformation should indicate the native resolution of the dataset. (If the top-level transform is not scale-and-translation-only, then we would not be able to infer a native resolution.)
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.
Thanks for the feedback @thewtex and @jbms. I had similar use-cases as the registration one in mind when proposing the current structure. (But then I considered not introducing it in 0.4 and leaving it for the follow-up proposals to keep things simple, because with the current spec we can only express fairly simple transformations that won't get us all the way for many use-cases.) Anyway, I think given your comments there seems to be some consensus that having the multiscales:coordinateTransformations
will be useful, so we might introduce it now already. I will try to get some feedback from @bogovicj on this as well, how this factors in with his initial approach to the extended transformation spec.
@sbesson: yes, we could introduce transformations even a level higher (that apply to each multiscales image), but I would not go there for now and would hope that such use-cases would instead be covered by an upcoming collections spec.
Sorry for missing this in #57 @will-moore. But I am in favor of using nouns for all the transformations to be compatible with future versions (what's the verb for |
Hi all, I've added a schema for v0.4 and valid/invalid examples at #87. |
@constantinpape @will-moore in the documentation and examples, can we add the |
To elaborate on this, the effect of downsampling on the location of pixel centers is a degree of freedom in the downsampling routine. The multiscale metadata should not contain assumptions about the details of a downsampling routine, which mandates both scale and translation transforms for each scale level. |
@d-v-b @thewtex Regarding pixel centers, the "shift" also depends on whether integer pixel coordinates are on the boundary between two pixels or in the center of a pixel. That should also be defined by this specification in order to specify the meaning of the transformations. Note: Neuroglancer defines the coordinate space such that integer pixel coordinates are on the boundary between two pixels. |
Yes, we do not have to make mandates on how downsampling is done. In the canonical examples for the most common use case of generating a multi-scale representation, it would be good to have the appropriate translations to help the reader's understanding.
Yes, we should clarify how a transformation applies to pixels. The center of a pixel is very strongly preferred because it is invariant to the spacing between pixels. It also means, for example, that pixels can be treated like points, #65 , etc. and the transformation is applied in the same way. |
In 2D applications using one of the pixel corners as origin is probably more common, but in 3D imaging voxel coordinate system origin is typically the center. In VTK, ITK, and all applications based on these toolkits origin is in the pixel center. In all 3D image file formats that I know of (nrrd, nifti, metaio, dicom) origin is in the pixel center, too. NGFF standard must specify this (if it hasn't been specified it already). If any software that uses different coordinate system convention internally then it can convert to/from the standard coordinate system when reading/writing images. |
@jbms @d-v-b @lassoan @thewtex Regarding examples: I agree that a case with |
@@ -236,9 +237,6 @@ Additional fields for the entry depend on "type" and are defined by the column ` | |||
| `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. | |
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.
It looks like this table still isn't rendered correctly:
I don't understand why. Could someone with more bikeshed experience give this a shot? cc @joshmoore @will-moore @sbesson
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.
I think I tried this way back on the previous PR, but didn't get much joy. I think @joshmoore suggested using an HTML table?
I am in favor of keeping the "multiscales:coordinateTransformations" for the reasons that @thewtex described. I agree with @thewtex and @lassoan re pixel centers, and so would be happy to say that explicitly without additional comment for Thanks @constantinpape ! |
Thanks for the comment @bogovicj. Given this feedback, let's keep the "multiscales:coordinateTransformations" in. |
This PR is good to go from my side now. @sbesson feel free to merge once you think it's good to go or ping me if you discover any more issues. |
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.
Thanks @constantinpape, the changes proposed match the discussions that happened as part of the last OME-NGFF meeting. Having re-read it carefully, I do not see any outstanding issue so I think it is a great time to freeze the proposed changes and move towards the 0.4 release.
Implementations and real-world examples can help exercising the new axes and transformations concepts introduced as part of this body of work. All feedback can be captured and reviewed together with the next body of work on transformations that will be led by @bogovicj.
Following up on this suggestion from @thewtex:
I would like to discuss this global coordinate transformation, because the example given in the 0.4 spec confused me: "multiscales" : {
...
"datasets": [
{
"path": "0",
"coordinateTransformations": [{"type": "scale", "scale": [1.0, 1.0, 0.5, 0.5, 0.5]}] # the voxel size for the first scale level (0.5 micrometer)
}
{
"path": "1",
"coordinateTransformations": [{"type": "scale", "scale": [1.0, 1.0, 1.0, 1.0, 1.0]}] # the voxel size for the second scale level (downscaled by a factor of 2 -> 1 micrometer)
},
{
"path": "2",
"coordinateTransformations": [{"type": "scale", "scale": [1.0, 1.0, 2.0, 2.0, 2.0]}] # the voxel size for the second scale level (downscaled by a factor of 4 -> 2 micrometer)
}
],
"coordinateTransformations": [{"type": "scale", "scale": [0.1, 1.0, 1.0, 1.0, 1.0]], # the time unit (0.1 milliseconds), which is the same for each scale level
...
} In this example, we have two different instances of the same transform applied to each scale level -- the global transform, and the scale-level-specific transform. Which one is correct? Should they be composed? In what order (for two scaling transforms the order doesn't matter...in which case why do we see two scaling transforms in this example?) I suggest requiring that, for each @thewtex raised the concern that all scale levels might share some large Curious to hear people's thoughts here. Personally I find this example so confusing that I think it needs to be clarified / amended. cc @bogovicj |
To me it seems natural that we have the following coordinate spaces:
The In cases where all scales other than the first (base level) are just computed from the base level via downsampling, it may be desirable to use the You do raise another issue, though. If we allow Note: it is not clear to me if the current spec allows coordinate transformations to be specified for a plain array --- I was under the impression that coordinate transformations could only be specified as part of the multiscale metadata. There is also the question of whether a dataset in one multiscale group could actually itself be a |
Coordinate transformations are defined for a single array (and I think this is their intended use) -- I believe this is covered here in the spec, although there is no example, and the introductory language here is a little confusing, i think instead of
I think if each dataset in
I agree that this is somewhat convenient, but I think the value of being explicit outweighs the burden in this case. |
(and no discussion of multiscale metadata is complete without a reference to the classic issue: zarr-developers/zarr-specs#50) |
Thanks for raising this concern @d-v-b, a couple of quick comments from my side.
They should be composed, the transformation in The comment by @jbms summarizes the motivation for having this pretty well, with the caveat that we don't have
The idea to give the common timepoint scaing in
I think you are mixing up a couple of things here. First of all in the zarr spec
Ngff is currently not using Furthemore,
There are multiple options to go forward:
Finally, I would suggest to move the discussion from this closed PR to either separate issues / PRs for specific problems or the future transformation discussions for general comments on transformations. |
Quite right! This lack of basic zarr knowledge reflects just how much time I have spent working with n5s :) And I will move this discussion to a separate issue. |
Follow up on #57:
transformations
tocoordinateTransformations
, after discussion in todays meeting with @bogovicj @jbms @lassoan and othersaxisIndices
for simplification. Transformations for a subset of axes will be tackled in future work.Other points from #57 that should still be addressed:
scale
andtranslation
, raised by @will-moore. We actually restrict this to firstscale
, thentranslation
; I will leave a code comment to point this out.Finally, we may want to remove the transformations applied to all transformations levels; I think that this introduces unnecessary complexity without offering much functionality. (Edit: on second thought we may want to keep it because this will be very useful once we have coordinateTransformations for subsets of axes)
@joshmoore @sbesson @will-moore @bogovicj please review with current implementation in ome-zarr-py / ome-napari, vizarr and the upcoming advanced transformation proposal in mind. Review from everyone else is of course also welcome, but please keep in mind that we don't want to introduce fundamental changes at this point for v0.4; these discussions can happen for the upcoming advanced transformation proposal that will be spearheaded by @bogovicj.