ome / ngff

Next-generation file format (NGFF) specifications for storing bioimaging data in the cloud.
https://ngff.openmicroscopy.org
Other
119 stars 41 forks source link

RFC-5: Transformations and Coordinate systems #255

Closed bogovicj closed 1 month ago

github-actions[bot] commented 3 months ago

Automated Review URLs

normanrz commented 3 months ago

Thanks for your continued work on the transformations proposal and for writing it up as an RFC. OME-Zarr really needs expanded transformation capabilities and I like this proposal! Happy to endorse it.

One issue that I see is the use of absolute paths in the path attribute (i.e. starting with /). This seems somewhat ill-defined to me. The example in the RFC shows that "store.zarr" is the root, which I assume would be the starting point for the absolute paths. However, any group in a Zarr hierarchy is a valid hierarchy on its own and there is no mandate for a .zarr suffix to denote a root. Therefore, I would advocate for restricting paths to relative paths, which would be relative to the folder that holds the zarr.json.

Also, I want to mention that there is interest in the community to create a "collections" proposal for OME-Zarr. I haven't gotten the time to write an RFC yet but I think that could integrate nicely with this transformations proposal and might provide solutions for the path issue as well.

LucaMarconato commented 2 months ago

Thanks @bogovicj for opening the draft for the RFC, I have added my review. The points are mostly minor and I have only two more small comments, here below. After the above is resolved and the comments are addressed, for me we are good to go to officially submit the RFC.

  1. I find this example of multiscale transformations vs "standard" transformations, very useful: https://github.com/bogovicj/ngff/blob/coord-transforms/latest/examples/multiscales_strict/multiscales_example_relative.json. I would stress this example more in the specs text.
  2. Some things are not clear to me from the example above: 2.1 are we allowed to use just scale, scale+translation or any transformation for the multiscale images? I am not sure what's the best here 2.2 related to the point above is this issue of "what is a multiscale image and what not" https://github.com/ome/ngff/issues/260. I opened it outside the context of this PR, but I think it is still useful to decide how to proceed.

Regarding 2.1, I think that we should allow for scale + translation and not general transformations. Also, I think a scale alone is now enough because we may need small translations depending of the downsampling algorithm because the pixel 0 of a downscaled image may not correspond to the pixel 0 of the original image. Also some people may find useful to define a image origin.

bogovicj commented 2 months ago

Thanks @LucaMarconato and @d-v-b ,

I address most of your comments. While there are few left, I'll get to them soon, and they're more minor. So I'm going to take this PR off of draft status.

joshmoore commented 1 month ago

Merging and sending for review once the build is green. If you would like to be added as an Endorser, please comment below or as always, PRs welcome.

Thanks to everyone involved! :tada:

tischi commented 6 days ago

I endorse this RFC