Lift 5d requirement for images and move multiscales description into spec #39

constantinpape · 2021-03-13T14:50:18Z

As discussed in #35 this lifts the restriction for images to be 5-dimensional.

Also, I have added a description of the multiscales metadata based on https://forum.image.sc/t/multiscale-arrays-v0-1/37930.
This will allow us to keep track of changes to the metadata better.

Note that I have added the field "axes" in order to specify the names of axes in the dataset; this becomes necessary due to
images not having fixed dimension any more. Note that there is still an ongoing discussion about the allowed names for axes (and also about allowing more than 5 dimensions) in #35 and #28. I have not addressed these issues yet, so for now the names are restricted to "t", "c", "x", "y", "z". Also, this is a SHOULD requirement to not break compatibility with 0.1.

@joshmoore

I am still refering to the version with 0.1, should it be bumped due to the changes w.r.t. number of dims?
Do I need to render the changes locally or is there some CI that also renders PRs? (Would be very convenient :))

joshmoore · 2021-03-14T10:20:27Z

I am still refering to the version with 0.1, should it be bumped due to the changes w.r.t. number of dims?

Yes. You can bump to an 0.2.x but if we go through with #29 (comment) it'll likely become an 0.3.x.

is there some CI that also renders PRs? (Would be very convenient :))

I haven't build it into the GHA workflow(s) yet (wink, wink) but:

http://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/ome/ngff/9fae8451eba2e3dbec45fe22c4a74bc66750831e/latest/index.bs produces the HTML that merging this PR would generate
while https://services.w3.org/htmldiff?doc1=https%3A%2F%2Fngff.openmicroscopy.org%2Flatest%2F&doc2=http%3A%2F%2Fapi.csswg.org%2Fbikeshed%2F%3Furl%3Dhttps%3A%2F%2Fraw.githubusercontent.com%2Fome%2Fngff%2F9fae8451eba2e3dbec45fe22c4a74bc66750831e%2Flatest%2Findex.bs even produces a diff of the existing latest/index.html to the output of this PR

sbesson

Thanks @constantinpape. A few initial questions largely revolving around the contract for the axes metadata.

sbesson · 2021-03-14T13:24:04Z

latest/index.bs

+It SHOULD contain the field "version", which indicates the version of the
+multiscale metadata of this image (current version is 0.1).
+
+It SHOULD contain the field "axes", which is a list of dimension names of the axes.


What is the default behavior if axes is not specified? In particular should the consumer have an expectation with regards to the number of underlying dimensions or try to detect it?

Alternatively, the lifting of the 5D requirement could lead to increasing the requirement for axes at a MUST level.

Yes, maybe it's better to change axes to MUST. I went for SHOULD to stay compatible with 0.1; but if hierarchical storage becomes MUST (see #29) 0.1 will be deprecated anyway and we can also promote axes to MUST.

Any thoughts @joshmoore?

No immediate objection to breaking now along with this loosening of the 5D/strict-ordering version, but it might be nice as a part of this effort to plan out the next 1 or 2 large changes based on #35 and see which are also going to be breaking.

No immediate objection to breaking now along with this loosening of the 5D/strict-ordering version, but it might be nice as a part of this effort to plan out the next 1 or 2 large changes based on #35 and see which are also going to be breaking.

Good point. I will try to summarise the current discussion in there later so we can see what else down the line might be breaking.

I changed to MUST now, also summarised potential changes: #35 (comment). As far as I can see none of this would be breaking with the current proposal for 0.2.

latest/index.bs

constantinpape · 2021-03-14T15:52:15Z

Yes. You can bump to an 0.2.x but if we go through with #29 (comment) it'll likely become an 0.3.x.

Ok, I will bump to 0.2 then. And looking forward to having hierarchies :).

I haven't build it into the GHA workflow(s) yet (wink, wink) but:

* http://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/ome/ngff/9fae8451eba2e3dbec45fe22c4a74bc66750831e/latest/index.bs produces the HTML that merging this PR would generate

* while https://services.w3.org/htmldiff?doc1=https%3A%2F%2Fngff.openmicroscopy.org%2Flatest%2F&doc2=http%3A%2F%2Fapi.csswg.org%2Fbikeshed%2F%3Furl%3Dhttps%3A%2F%2Fraw.githubusercontent.com%2Fome%2Fngff%2F9fae8451eba2e3dbec45fe22c4a74bc66750831e%2Flatest%2Findex.bs even produces a diff of the existing latest/index.html to the output of this PR

Cool, I will check it out!

joshmoore

Few thoughts on top of Seb's comments.

latest/index.bs

joshmoore · 2021-03-16T10:16:24Z

latest/index.bs

-    │   ├── .zarray           # All image arrays are 5-dimensional
-    │   │                     # with dimension order (t, c, z, y, x).
+    │   ├── .zarray           # All image arrays are up to 5-dimensional with dimension order (t, c, z, y, x).
+    │   │                     # An image without time dimension has axes (c, z, y, x) etc.


Is a 3D array interpreted as (z, y, x)?

If we promote "axes" to MUST, then the interpretation always depends on the metadata, i.e. on the values in "axes".

latest/index.bs

jni · 2021-03-17T08:14:26Z

Hi all,

Sorry to go around in circles, but I'd prefer relaxing MUST to SHOULD here. In the absence of axis labels, consumers could process things as:

5D: backwards-compatible behaviour, TCZYX
Anything else: all spatial dimensions unless time or channel axes are specified by the consumer.

Of course, the latter behaviour is asking for trouble, but that's what SHOULD means.

My two cents. Of course, since backwards compatibility is no object given the versioning scheme, I am not too stressed about this choice. 😊 Thanks @constantinpape for jumping in!

constantinpape · 2021-03-18T10:14:45Z

@jni I would still opt for MUST, because not having "axes" is inviting undefined behaviour and we will also require to have this
in future iterations of the spec, see discussion in #35. So requiring it now is also more future proof.

* Anything else: all spatial dimensions unless time or channel axes are specified _by the consumer_.
Of course, the latter behaviour is asking for trouble, but that's what SHOULD means.

Consumer software can still treat this metadata as a hint and over-ride it or allow the user to override; or even dismiss it completely.

joshmoore · 2021-03-19T07:41:27Z

FWIW xarray's use of zarr requires an _ARRAY_DIMENSIONS attribute: http://xarray.pydata.org/en/stable/internals.html#zarr-encoding-specification

see also: pydata/xarray#4118 (comment) cc: @rabernat

jni · 2021-03-19T10:45:46Z

@constantinpape

Consumer software can still treat this metadata as a hint and over-ride it or allow the user to override; or even dismiss it completely.

I'm not worried about consumers, but writers/converters. Many image formats don't have well-defined axis names, so if I want to convert some TIFF or .npy to NGFFv0.2, I now have to make up some axes. Which I guess is fine, zyx is a reasonable guess as I point out above, but it feels yuckier at write time (I'm codifying my guess to disk) than at read time (my guess is ephemeral until I close the software).

d-v-b · 2021-03-19T14:46:27Z

this is a problem for xarray, which it solves by using default names dim_0, dim_1, etc. Those names seem a little more generic than x,y,z

constantinpape · 2021-03-20T17:10:43Z

this is a problem for xarray, which it solves by using default names dim_0, dim_1, etc. Those names seem a little more generic than x,y,z

But the xarray scope is also a more general than NGFF. I think x, y, z, c, y will cover most of the use case AND these changes are just an intermediate step until we can agree on a more general solution (that may also involve allowing more than 5d), see discussion in #35.
We need to make sure though that what we decide here does not require breaking changes in a more general solution, see #35 (comment).

joshmoore · 2021-03-25T14:17:15Z

@tlambert03 just pinged me elsewhere that this is a general improvement in terms of readability (e.g. "how does one store an image with only one resolution?" Answer: "as a multiscales of array length = 1")

joshmoore · 2021-03-29T10:55:45Z

Are there any other high-level points, or is the discussion now primarily around whether this is a MUST or a SHOULD?

If so, I'm inclined to bump this to 0.3 (due to #40) and get it in as "latest" while we start on implementations.

constantinpape · 2021-03-29T11:04:04Z

Are there any other high-level points, or is the discussion now primarily around whether this is a MUST or a SHOULD?

Yes, I think this is the only open point of discussion.

If so, I'm inclined to bump this to 0.3 (due to #40) and get it in as "latest" while we start on implementations.

👍

constantinpape · 2021-04-14T21:45:43Z

Let's revitalize this PR. I think there are only 2 things to do before this can be merged:

Do we classify axes as MUST or SHOULD?
- As mentioned above, I think the changes here are only a first step towards Data dimensionality and axes metadata #35. So I don't have a very strong preference here and SHOULD would also be fine with me.
How do we coordinate with 0.2.0: switch to nested layout #40? Either of the options below is fine with me.
- Merge this PR before it
- Merge 0.2.0: switch to nested layout #40 and then merge this one, fixing potential merge conflicts.
- Rebase this PR on 0.2.0: switch to nested layout #40, merge it and then merge into main

joshmoore · 2021-04-16T12:36:59Z

Do we classify axes as MUST or SHOULD?

I'll just add a data point to this discussion for now: https://twitter.com/shoyer/status/1379877702254034946

How do we coordinate with 0.2.0: switch to nested layout #40?

@constantinpape, if you can prepare a commit which is the information from zarr-developers/zarr-specs#50 without the specific bits of this PR, then I'm happy to cherry-pick and get 0.2.0 out the door. Then you can move this PR to 0.3.0.

constantinpape · 2021-04-26T08:57:28Z

Thanks for the reviews everyone.
I will close this PR to not get confused; we will first incorporate the wording changes into 0.2 and then work on adding axes.

joshmoore · 2021-04-27T06:50:35Z

This PR has been migrated to #46

Please join the conversation there. It would be good to get a first version of axes naming in place.

constantinpape added 2 commits March 13, 2021 15:04

Lift 5d requirement for images

e1a5168

Move multiscales definition into spec

9fae845

constantinpape requested a review from joshmoore March 13, 2021 14:50

constantinpape mentioned this pull request Mar 13, 2021

Data dimensionality and axes metadata #35

Closed

sbesson reviewed Mar 14, 2021

View reviewed changes

joshmoore reviewed Mar 16, 2021

View reviewed changes

Address reviews: make axes MUST; bump version to 0.2

4223549

will-moore reviewed Mar 16, 2021

View reviewed changes

latest/index.bs Outdated Show resolved Hide resolved

will-moore reviewed Mar 16, 2021

View reviewed changes

latest/index.bs Outdated Show resolved Hide resolved

Address review comments

7b664a9

glyg mentioned this pull request Mar 22, 2021

attempts to create a writer ome/ome-zarr-py#69

Merged

sbesson mentioned this pull request Mar 29, 2021

0.2.0: switch to nested layout #40

Merged

constantinpape closed this Apr 26, 2021

constantinpape mentioned this pull request Apr 26, 2021

Add description of multiscales metadata 0.2 #44

Merged

joshmoore mentioned this pull request May 5, 2021

Compatibility with xarray #48

Open

constantinpape mentioned this pull request May 5, 2021

Add axes field to multiscale metadata #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lift 5d requirement for images and move multiscales description into spec #39

Lift 5d requirement for images and move multiscales description into spec #39

constantinpape commented Mar 13, 2021

joshmoore commented Mar 14, 2021 •

edited

Loading

sbesson left a comment

sbesson Mar 14, 2021

constantinpape Mar 14, 2021

constantinpape Mar 14, 2021

joshmoore Mar 16, 2021 •

edited

Loading

constantinpape Mar 16, 2021

constantinpape Mar 16, 2021

constantinpape commented Mar 14, 2021

joshmoore left a comment

joshmoore Mar 16, 2021

constantinpape Mar 16, 2021

jni commented Mar 17, 2021

constantinpape commented Mar 18, 2021

joshmoore commented Mar 19, 2021

jni commented Mar 19, 2021

d-v-b commented Mar 19, 2021

constantinpape commented Mar 20, 2021

joshmoore commented Mar 25, 2021

joshmoore commented Mar 29, 2021

constantinpape commented Mar 29, 2021

constantinpape commented Apr 14, 2021

joshmoore commented Apr 16, 2021

constantinpape commented Apr 26, 2021

joshmoore commented Apr 27, 2021

Lift 5d requirement for images and move multiscales description into spec #39

Lift 5d requirement for images and move multiscales description into spec #39

Conversation

constantinpape commented Mar 13, 2021

joshmoore commented Mar 14, 2021 • edited Loading

sbesson left a comment

Choose a reason for hiding this comment

sbesson Mar 14, 2021

Choose a reason for hiding this comment

constantinpape Mar 14, 2021

Choose a reason for hiding this comment

constantinpape Mar 14, 2021

Choose a reason for hiding this comment

joshmoore Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

constantinpape Mar 16, 2021

Choose a reason for hiding this comment

constantinpape Mar 16, 2021

Choose a reason for hiding this comment

constantinpape commented Mar 14, 2021

joshmoore left a comment

Choose a reason for hiding this comment

joshmoore Mar 16, 2021

Choose a reason for hiding this comment

constantinpape Mar 16, 2021

Choose a reason for hiding this comment

jni commented Mar 17, 2021

constantinpape commented Mar 18, 2021

joshmoore commented Mar 19, 2021

jni commented Mar 19, 2021

d-v-b commented Mar 19, 2021

constantinpape commented Mar 20, 2021

joshmoore commented Mar 25, 2021

joshmoore commented Mar 29, 2021

constantinpape commented Mar 29, 2021

constantinpape commented Apr 14, 2021

joshmoore commented Apr 16, 2021

constantinpape commented Apr 26, 2021

joshmoore commented Apr 27, 2021

joshmoore commented Mar 14, 2021 •

edited

Loading

joshmoore Mar 16, 2021 •

edited

Loading