Skip to content
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

simplify multiscale and examples #172

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

d-v-b
Copy link
Contributor

@d-v-b d-v-b commented Mar 10, 2023

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
Copy link
Contributor

github-actions bot commented Mar 10, 2023

Automated Review URLs

@bogovicj
Copy link
Contributor

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.

@clbarnes
Copy link

clbarnes commented Jul 5, 2023

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
Copy link
Contributor Author

d-v-b commented Jul 8, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants