simplify multiscale and examples

d-v-b commented 1 year ago

This PR simplifies the multiscale metadata. A few highlights:

No more top-level coordinateTransformations that apply to all elements of datasets. This is unneeded, because we can convey the exact same information via the coordinateTransformations field for each element of datasets.
scale and translation transformations are required for each element of datasets. Reasoning for this decision is added to the spec.
the type field of multiscales is removed. A better place for that information is inside the metadata field.
the version field of multiscales is now required. If this is optional, it causes headaches for implementations / parsers, and the version should always be defined anyway, so we should mandate it.
the types of some fields were made more explicit

github-actions[bot] commented 1 year ago

Automated Review URLs

bogovicj commented 1 year ago

the type field of multiscales is removed. A better place for that information is inside the metadata field.

Agreed.

the version field of multiscales is now required.

Agreed.

relation to coordinate systems

No more top-level coordinateTransformations that apply to all elements of datasets.

scale and translation transformations are required for each element of datasets. Reasoning for this decision is added to the spec.

These two both interact with coordinate systems (CS) https://github.com/ome/ngff/pull/138 . Some of the details of your changes won't be applicable after CS gets merged (e.g. there are no longer explicitly "top level" coordinateTransformations (CT)), but there can still be implicitly:

multiple "output" coordinate systems?

There is a need to display / work with the same array data in different coordinate systems. One of my goals in the CS / CT PR was to make that clear. A multiscales can have an array of coordinate systems, applications can let users select one or default to the first one. Analogous to the fact that multiscales points to an array, each element of which might have different datasets.

So while there are indeed "No more top-level coordinateTransformations that apply to all elements of datasets.", a related idea is needed.

constraints on `coordinateTransformation` in `datasets`

I aimed to put as few constraints as possible in the CS/CT PR, but I think there should be some. I'd want to rephrase your ideas into constraints in my PR, but it may not be so simple, for example:

`affine`

Here: https://github.com/d-v-b/ngff/blob/94fadb46eb9f4f50565c344b0b576e2730ae559a/latest/examples/multiscales_strict/multiscales_example.json#L27-L34

Do you think it should be valid to instead use:

"coordinateTransformations": [{

    "type": "affine",
    "affine": [0.1, 0.0, 0.0, 0.0, 0.0, 0.0,
               0.0, 1.0, 0.0, 0.0, 0.0, 0.0,
               0.0, 0.0, 1.0, 0.0, 0.0, 0.25,
               0.0, 0.0, 0.0, 1.0, 0.0, 0.25,
               0.0, 0.0, 0.0, 0.0, 1.0, 0.25 ]
}]

My inclination is for it to be valid but not recommended.

clbarnes commented 1 year ago

No more top-level coordinateTransformations that apply to all elements of datasets. This is unneeded, because we can convey the exact same information via the coordinateTransformations field for each element of datasets.

Agreed.

scale and translation transformations are required for each element of datasets. Reasoning for this decision is added to the spec.

As John says, this gets more complicated when more types of transform are added (as do order constraints). If we're only ever going to use scale and translation, a convenient way to represent it would be "coordinateTransformations": {"scale": [1.0, 1.0], "translation": [0.0, 0.0]}.

the type field of multiscales is removed. A better place for that information is inside the metadata field.

Agreed.

the version field of multiscales is now required. If this is optional, it causes headaches for implementations / parsers, and the version should always be defined anyway, so we should mandate it.

Agreed.

the types of some fields were made more explicit

Agreed.

d-v-b commented 1 year ago

the type field of multiscales is removed. A better place for that information is inside the metadata field.

Agreed.

the version field of multiscales is now required.

Agreed.

relation to coordinate systems

No more top-level coordinateTransformations that apply to all elements of datasets.

scale and translation transformations are required for each element of datasets. Reasoning for this decision is added to the spec.

These two both interact with coordinate systems (CS) #138 . Some of the details of your changes won't be applicable after CS gets merged (e.g. there are no longer explicitly "top level" coordinateTransformations (CT)), but there can still be implicitly:

multiple "output" coordinate systems?

There is a need to display / work with the same array data in different coordinate systems. One of my goals in the CS / CT PR was to make that clear. A multiscales can have an array of coordinate systems, applications can let users select one or default to the first one. Analogous to the fact that multiscales points to an array, each element of which might have different datasets.

So while there are indeed "No more top-level coordinateTransformations that apply to all elements of datasets.", a related idea is needed.

constraints on coordinateTransformation in datasets

I aimed to put as few constraints as possible in the CS/CT PR, but I think there should be some. I'd want to rephrase your ideas into constraints in my PR, but it may not be so simple, for example:

affine

Here: https://github.com/d-v-b/ngff/blob/94fadb46eb9f4f50565c344b0b576e2730ae559a/latest/examples/multiscales_strict/multiscales_example.json#L27-L34

Do you think it should be valid to instead use:
"coordinateTransformations": [{

    "type": "affine",
    "affine": [0.1, 0.0, 0.0, 0.0, 0.0, 0.0,
               0.0, 1.0, 0.0, 0.0, 0.0, 0.0,
               0.0, 0.0, 1.0, 0.0, 0.0, 0.25,
               0.0, 0.0, 0.0, 1.0, 0.0, 0.25,
               0.0, 0.0, 0.0, 0.0, 1.0, 0.25 ]
}]
My inclination is for it to be valid but not recommended.

Can you explain the problem with that affine transform? Is it just too much text or something? Because as long as consuming software can parse it, I don't see the problem. Someone could also throw in 100 chained identity transforms, but I don't think we need to put something in the spec suggesting that people refrain from this.

ome / ngff