RFC-3: Add Review 2 from Fideus Labs - #597
Conversation
Co-authored-by: Valentin Boussot <valentin@fideus.io>
Automated Review URLs |
|
this is great! i had 2 inline comments about logical axis order and dimension limits |
@d-v-b please put these in your Comment or Review. |
|
|
||
| ### Normative `SHOULD` in rule 3 | ||
|
|
||
| Rule 3 reads "it should be named `t`", lowercase, whereas rules 1 and 2 use `SHOULD` and the Proposal paragraph says the time axis "SHOULD have name "t" and type "time"". Since the specification reads these key words per RFC 2119, the lowercase form is non-normative once inserted. We ask for `SHOULD`. |
There was a problem hiding this comment.
Sorry can you clarify this? I don't understand. Are you saying that the recommendation should specify a lowercase "t" explicitly?
There was a problem hiding this comment.
Ah sorry, badly worded on my side! Nothing about t, which is perfect. I meant the word "should": rules 1 and 2 use the RFC 2119 SHOULD, rule 3 spells it lowercase, so in the spec it would read as plain prose instead of a recommendation. Capitalizing it is the whole ask.
|
|
||
| ### Rule 6 is not testable for custom axes | ||
|
|
||
| Rule 6 requires that "the order of the axes MUST match their ordering within the data if applicable". For `coordinate` and `displacement` axes this matches what RFC-5 already defines ("the `i`th value of the array along the `coordinate` or `displacement` axis refers to the `i`th output axis"). For the other axes the RFC motivates, such as a length-6 diffusion tensor axis, Euler angles, or quaternions, nothing in the axes metadata defines what index 0 is, so there is no ordering to match and the MUST cannot be checked. We are not asking RFC-3 to standardize tensor layouts; we ask that rule 6 be scoped to axes whose component semantics the specification defines, or reworded as guidance. |
There was a problem hiding this comment.
I feel like "if applicable" is already doing the work requested here...
I disagree about diffusion tensors: if the image spatial axes have order zyx, then my diffusion tensor MUST look like:
[[Dzz, Dzy, Dzx],
[Dyz, Dyy, Dyx],
[Dxz, Dxy, Dxx]]
and MUST NOT look like:
[[Dxx, Dxy, Dxz],
[Dyx, Dyy, Dyz],
[Dzx, Dzy, Dzz]]
and vice-versa. There's multiple ways to encode the symmetric tensor as a 1D vector but my expectation is that the first and last element should be constrained by the language.
There was a problem hiding this comment.
Fair point on "if applicable", it does cover things like Euler angles or quaternions. What we're really asking is for the rule to say which axis types it applies to; today that's coordinate and displacement, where RFC-5 defines the mapping.
For the tensor as a matrix, your example is clear, agreed. It's the flattened single-axis form that worries me: first and last element don't pin it down. Dzz, Dzy, Dzx, Dyy, Dyx, Dxx and Dzz, Dzy, Dyy, Dzx, Dyx, Dxx both start with Dzz, end with Dxx, and follow zyx, yet they're different (C-style row-major vs LAPACK packed storage). So two writers can both satisfy the MUST and still disagree. Just naming the applicable types would fix it, no need for RFC-3 to standardize tensor packing.
There was a problem hiding this comment.
memory order vs dimension order has come up already, it would be helpful to get an explanation of what "ordering within the data" is intended to mean, as in this sentence:
"the order of the axes MUST match their ordering within the data if applicable"
There's the expectation (and need) that the list of axes declared in ome zarr metadata match something. Zarr V3 does not guarantee memory-order stability across implementations. So you can't require that ND arrays be decoded to C or F ordered memory, or even that they be decoded to contiguous memory at all, and so you can't require that any logical array element appear in any particular order in the in-memory representation of a decoded chunk. Implementations just have to agree on the logical order of dimensions, and that's given by the dimension_names field. This is a subtle point, and I get the feeling that some folks in the conversation are working with the assumption that there's a "default memory order". Maybe ome-zarr can impose one? but it would be an uphill battle against a core feature of the zarr v3 spec.
|
|
||
| ### The list of specification changes is not exhaustive | ||
|
|
||
| The Proposal states that "No further changes to the specification document are proposed by this RFC." The 0.6rc0 hierarchy diagram still says "All image arrays must be up to 5-dimensional with the axis of type time before type channel, before spatial axes"; the `coordinates` and `displacements` constraints still place the vector axis "after a time axis (if present) and before the spatial axes", which relies on the ordering RFC-3 removes; and the schemas keep the same limits (`axes.schema` `maxItems: 5` and two or three `space` axes, `mapAxis` and `projectAxis` indices bounded to 0..4, `rotation` matrices 2x2 to 5x5 only). The 0.9.dev1 draft ([ome/ngff-spec#190](https://github.com/ome/ngff-spec/pull/190)) had to change all of these and flags the `rotation` enumeration as "incompatible with RFC-3 but cannot be easily extended". We ask that the list be made exhaustive, text and schemas, or that the sentence be replaced by one that says consequential edits are expected and names the known ones. |
There was a problem hiding this comment.
Noted. This oversight is an artifact of the RFC being originally authored around the time 0.5 was released.
|
Thanks for the clarifying comments. Merging in expectation of @jni's response. |
6 participants
Co-authored-by: Valentin Boussot valentin@fideus.io