Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Extend the axes fields in multiscales metadata #57
Extend the axes fields in multiscales metadata #57
Changes from all commits
4703d89
e72ee48
212b2dd
df835f1
b3523c9
70ebede
a8597d7
8ca7976
77790b3
755eaf1
fa390f6
47620ed
ffcb2a8
e27bdec
0661115
23be956
0419ce1
e94a0c1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Looking at how this table is rendered at http://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/ome/ngff/47620eddae0d04a078505ae7189b7e0e94f5feed/latest/index.bs#trafo-md somethings not right
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.
Maybe just needs at least 3
---
below the header: https://www.markdownguide.org/extended-syntax/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 checking. I haven't looked at the rendering at all yet; will give it another look.
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 I have tried to fix this. How do I find the link for the latest deployment to check?
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 URL is in the README. Basically just replace the checksum e.g. http://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/ome/ngff/e27bdecc06dedc71c6ec7e20a2bd7dd0755d43ea/latest/index.bs#trafo-md
I think I failed to get this to work which is by the version table at the bottom uses
<table>
.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.
This seems like a lot of rules to handle the case where some axes don't need a scale.
I wonder if instead of
"scale": [0.5, 0.5, 0.5], "axisIndices": [2, 3, 4]
we could do"scale": [null, null, 0.5, 0.5, 0.5]
. Then we can simply say that the length ofscale
list must be equal to the number of dimensions. And we don't need to have all theaxisIndices
rules (and validation logic). It's also less code to find the scale for e.g. dimension 2.compared with:
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 - how would you know that those are the spatial axes? while a certain ordering is recommended it is not mandatory under the spec? the spec currently decouples dimension from type of 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.
Agree that the above case is shorter, but the code to generate a 3d -> 3d transform would be longer with the
[null, null, 0.5, 0.5, 0.5]
case. I.e. the code would have to check which dimensions are spatial, then slice the scale array appropriately.I don't have much of a preference between the two, honestly. But some things are easier if the transforms always consider dimensions of the same type.
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.
"scale": [null, null, 0.5, 0.5, 0.5]
is simply an easier way to store the info that is in"scale": [0.5, 0.5, 0.5], "axisIndices": [2, 3, 4]
. In both cases the spatial axes are 2, 3 and 4 (because they're notnull
in[null, null, 0.5, 0.5, 0.5]
.If you ONLY had spatial axes. e.g.
zyx
then it's simply"scale": [0.5, 0.5, 0.5]
.If the dimension order is different. e.g.
z, t, y, x
then use"scale": [0.5, null, 0.5, 0.5]
instead of"scale": [0.5, 0.5, 0.5], "axisIndices": [0, 2, 3]
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.
You don't need
null
for scale --- you can just use 1. And I agree thataxisIndices
just adds unnecessary complexity.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.
@bogovicj Can you explain a bit more why it is so important to be able to specify
axisIndices
?Do you imagine frequently writing out this metadata fully manually?
With the current state of this proposal, it appears that affine transforms aren't even supported --- only translation and scale. So there it is just a matter of specifying
0
or1
(for translation or scale, respectively) for any axes that are to be left alone --- it seems likeaxisIndices
would really only provide a benefit when manually writing out the metadata for a full affine transform (not yet part of the proposal, but presumably planned to be added).You mentioned the specific use case of translating x and y only. For the use case you have in mind, is the translation by an integer amount? If so, would that use case perhaps be better addressed by support for a non-zero origin in zarr?
zarr-developers/zarr-specs#122
For programmatic reading and writing of this metadata, a single affine transform would seem to be the most straightforward way to deal with these transformations. I suppose implementations that don't support anything but translation and scaling might instead normalize to a single translation vector and single scale vector. But they could just as well use an affine transform but return an error if the rotation matrix is not diagonal. It seems to me that composing of multiple transforms, and support for things like
axisIndices
, as specified in the current proposal, could instead by handled by library functions in the program used to generate the OME-zarr metadata.Likewise, in a graphical user interface a single affine transform would also seem to be the most straightforward way to present the transformation, as otherwise there is a lot of added complexity in displaying possibly multiple different transforms of different types, and making it clear in what order they are applied. In Neuroglancer affine transforms are shown in their normal matrix form but with the rows and columns labeled to indicate to which source/target dimension they correspond and which column corresponds to translation. That makes it fairly easy to understand even if you have more than 3 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.
@bogovicj Sorry, I didn't appreciate how this would work with affine transforms. I agree that your example with
axisIndices
looks simpler, although I don't actually understand either!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 quickly revive this conversation: as @bogovicj explains, we have introduced
axisIndices
to enable specifying transformations for a subset of the axes, e.g. for the use-case of affine transformations.I think having the option of specifying subsets for transformations is crucial to have in the transformations, but I agree that
axisIndices
may not be the best solution. I see a few potential options:scale={"x": 2, "y": 2}
(as discussed above). Names must be the same as given in the axes definition. In this case we don't needaxisIndices
axisIndices
from the current proposal and deal with how to address subset of axes later.axisIndices
are documentedMy personal preference would be option 1, because I think that this would be the most elegant solution.
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 nice summary @constantinpape .
I like option (1) as well.
I need to clarify this, so let me describe a simpler example as a case study. There is a fairly common use case in which we need to apply a nonlinear 2D transform to every plane of a 3D volume (e.g., lens correction).
This is not covered an offset or an affine. We like the
axisIndices
(or similar) idea here because it lets us store the transform in a straightforward way: as a 2D displacement field (a 3D volume:[2,X,Y]
, where the 2 dimension is a vector describing the displacements at[:,x,y]
).Without
axisIndices
(or some way to communicate "this transform is applied over a subset of the coordinates), then what is the right way to store the transform / communicate this idea? I guess by making it a 3D displacement field instead? So a[3,X,Y,Z]
array where the[2,:,:,:]
is full of zeros, and[0:1,:,:,z]
all contain identical data. That's wasteful.I'm not sure how to deal with this use case without being explicit about this "subset 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.
I would absolutely agree that having to store extra zeros would be unacceptable, and agree that something like axisIndices is very useful in the flow field case.
You could accomplish the same thing, in a somewhat more general way, by allowing to specify an affine transform that is applied to every flow vector. However, I'm not sure if that extra generality would actually be helpful.
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.
sequentially
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 would be good to specify precisely the mathematical formula in terms of transformed space coordinates and original coordinates. Otherwise there is a possibility for confusion as to the precise interpretation of the scale and translation values.
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 comment @jbms. I think this is a good point.
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.
Just trying to implement the handling of scale and translation in
napari-ome-zarr
. Napari supportsscale
andtranslate
metadata (see add_image). Napari doesn't specify an order, but it seems by testing that the translate values are in real-world values (applied after scaling).Do we want to specify the same for translation in this NGFF spec? Or does this depend on the ordering of
translation
andscale
? If thetranslation
comes beforescale
then it's in pixels. If after then it's in real space?Also, just a thought with naming. In napari it's
scale
andtranslate
(both verbs) but we havescale
andtranslation
. Do we want to usetranslate
instead?