Streamline input output constraints#137
Conversation
Automated Review URLs |
Co-authored-by: Benedikt Best <63287233+btbest@users.noreply.github.com>
…dinateTransformations
|
Also, I noticed that in #117 , I introduced a constraint that would require all transformations under cc @btbest @will-moore @clbarnes @tlambert03 @dstansby @kevinyamauchi |
|
Happy with loosening the directionality, but the current MAY phrasing accidentally reintroduces the "multiscale transforms can point anywhere" problem:
The MAY in the second bullet point is the catch. Maybe I just have mischief baked in, but I immediately read this as "Well, you never told me I can't reference some other multiscale or a scene". This should say "The other MUST reference a coordinate system either in... or in..." (the same goes for line 1328) |
Right! |
… is not a MAY Co-Authored-By: Benedikt Best <63287233+btbest@users.noreply.github.com>
b2e14de to
4880937
Compare
|
@tlambert03 would you mind you have a quick look at this branch and see if you find the wording around the constraints around the |
There was a problem hiding this comment.
Looks good as is, I just added some minor phrasing suggestions.
Remembering that any multiscales group can technically define multiple multiscales actually presents a semantic problem: the name + path referencing isn't ambiguous if the multiscales group at the path defines multiple multiscales whose intrinsic systems are identically named. I doubt this would be an issue in practice because nobody uses multiple entries in the multiscales array, but it's theoretically there.
I think to have version 0.6 being clean, we'd need the "The multiscales array MUST contain exactly one multiscale object` constraint that has been discussed before (but I think got delayed because it was bundled with flattening the multiscales-list to a single object, making it a breaking change for almost 0 gain).
| that refer to coordinate systems in the same zarr.json or in the metadata of multiscale image subgroups. | ||
| - One of `input` or `output` MUST reference the "intrinsic" coordinate system by `name` (`path` MAY be omitted or null). | ||
| - The other MUST reference a named coordinate system in the same multiscales group (by `name`, `path` MAY be omitted or null), or in a child [labels](#labels-md) group (by `name` and `path`). | ||
| - When referencing a child labels group, the transformation MUST be [`identity`](#identity-md), [`scale`](#scale-md), or [`translation`](#translation-md). |
There was a problem hiding this comment.
Need to check again but aren't labels required to be the same shape as the multiscale; which means this basically always has to be identity? Allowing scale and translation isn't a problem of course, just presents more opportunities for confusion I suppose.
There was a problem hiding this comment.
That's the part where we are entering the grey zone between collections and transforms. The relationship between labels and images is first and foremost semantic in nature, not spatial. Hence 0.6 will not force you to add spatial information to your label/image connection, but if you want to, you can.
Hence, the shaping of the labels has never been written out in previous versions and 0.6 won't force this because only collections will express this relationship properly.
There was a problem hiding this comment.
The relationship between labels and images is first and foremost semantic in nature, not spatial
Subtle disagreement from the spec itself there:
Some applications–notably image segmentation–produce a new image that is in the same coordinate system as a corresponding multiscale image (usually having the same dimensions and coordinate transformations).
Sounds very much spatial to me :)
Just to be clear, I'm not actually asking for anything to be changed here. Just indulging.
There was a problem hiding this comment.
Oh, I see what you mean. The loophole here is "usually"; There was a PR where thought of changing that (possibly #117) and require this transformation to be explicit. Ultimately we decided against it:
- If that transformation were to live in the parent multiscale image, then the addition of a child
labelsimage would effectively invalidate the parent image because that transform would then be missing - If the transform were to live in
/labels/segmentation_image/zarr.json, its output would have to be../../zarr.json, which would also be problematic because moving the label image elsewhere would make it invalid and thus no longer self-describing.
The clean option would be to put the label image into a transformation graph and not use the /labels/... structure at all. Or wait for collections that will make this kind of linkage much more explicit from the point of view of an apex zarr.json in the hierarchy, which would be my preferred solution :)
There was a problem hiding this comment.
I also don't see any point to requiring the metadata to enumerate all contents. At most I'd go to "SHOULD" level - just like the labels metadata only "SHOULD" reference all label multiscales available.
Gulp...there's an entire RFC around this (with a complicated history, I think ^^"). Essentially, it was decided to also defer to this to collections, because collections will arrange this bit of metadata anyway. As you say, so far there haven't really been any datasets in the wild that would make use of the array structure under the |
Co-authored-by: Benedikt Best <63287233+btbest@users.noreply.github.com>
Co-authored-by: Benedikt Best <63287233+btbest@users.noreply.github.com>
Co-authored-by: Benedikt Best <63287233+btbest@users.noreply.github.com>
This reverts commit 96d4f2c.
|
Follow-up: Since we are using the term |
|
I agree that this statement would be the cleanliest way out of that issue:
But given the discussion around it, I think it's safer to just let it rest as it is for the moment and save the cleaning up for 1.0 😬 |
Fixes ome/ngff#523
Fixes #159
ms > coordinateTransformations, which makes the text cleaner and has no practical implication since we are only allowing identity/scale/translate/sequence here.cc @btbest