-
Notifications
You must be signed in to change notification settings - Fork 75
RFC-3 response to review 2 #614
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
Open
jni
wants to merge
10
commits into
ome:main
Choose a base branch
from
jni:rfc-3-response
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
848519d
Update proposed axis naming rules to include type
jni f8e094e
Clarify w must not be both time and space frequencies
jni eea5035
should -> SHOULD
jni 92b1a37
uniqueness within coord system
jni 82f8acd
More specific example for coordinate ordering
jni b469cc1
Enumerate additional changes to the spec
jni 0923718
Fix outdated forward-compatibility
jni de7093d
Update testing section
jni 7f13299
Add response
jni 48e3a9d
Update changelog
jni File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| # RFC-3: Response 1 | ||
|
|
||
| ## Summary of Changes | ||
|
|
||
| In response to review 2, RFC-3 has been updated to clarify certain | ||
| recommendations and add new ones, to help implementers make decisions. | ||
|
|
||
| ## Review feedback | ||
|
|
||
| [Review 2](rfcs:rfc3:review2) provided a convenient checklist of all the | ||
| recommendations. It is included below for cross-reference, with responses or | ||
| resulting changes noted inline. | ||
|
|
||
| ### Minor changes | ||
|
|
||
| - [ ] Add a statement that, if present, `t`, `c`, `z`, `y`, `x` axes SHOULD | ||
| occur in that order. | ||
| - We disagree with this recommendation, as one of the key motivations of | ||
| RFC is freedom to reorder these axes when acquiring data, or when | ||
| performing different analyses that may require different access | ||
| patterns. While the recommended ordering is optimal in many scenarios, | ||
| we continue to stress that documentation is the right place for this | ||
| recommendation, not the NGFF specification. | ||
| - [x] Carry the `type` recommendation into rules 1 to 3 and provide guidance | ||
| and examples on how and what to name custom types. | ||
| - Done in 848519d. Note that we have renumbered rules to start from 1, so | ||
| these are now 2 to 4. | ||
| - [x] Resolve the naming collision between the spatial and temporal frequency | ||
| axes in rule 4 so that rules 4 and 5 can both be satisfied. | ||
| - Done in f8e094e. | ||
| - [x] Use `SHOULD` in rule 3. | ||
| - Done in eea5035. | ||
| - [x] Say "within the same coordinate system" in rule 5, or state and reconcile | ||
| the stronger scope. | ||
| - Done in 92b1a37. | ||
| - [x] Scope rule 6 to axes whose component semantics the specification defines, | ||
| or reword it so the MUST can be checked. | ||
| - Rule 6 (now 7) includes the phrase "if applicable". Because we can't | ||
| anticipate all the data types where it would apply, we intentionally | ||
| leave room for interpretation. Having said that, the reviewers | ||
| correctly pointed out that the recommendation for displacement fields | ||
| was redundant with RFC-5. However, the recommendation applies more | ||
| generally than in the context of coordinate transformations. We have | ||
| therefore changed the example to a vector field representing particle | ||
| or fluid motion. | ||
| - [x] Make the list of specification changes exhaustive (hierarchy comment, | ||
| RFC-5 vector-axis placement wording, schema limits), or remove the "no | ||
| further changes" sentence and enumerate the known edits. | ||
| - Done in b469cc1. | ||
| - [x] Update Forward Compatibility to the 0.6rc0 and RFC-5 state and name the | ||
| remaining dimensional assumptions. | ||
| - Done in 0923718. | ||
| - [x] Distinguish dataset conformance from partial implementation support, and | ||
| cover the new MUST rules in the shared ngff-spec conformance cases, with | ||
| checks outside the schema for name uniqueness and rule 0. | ||
| - I have clarified the text in the testing section (de7093d). Additional | ||
| test cases in ome-ngff will be added after the 0.9.dev2 cleanup | ||
| ([ome/ngff-spec#201](https://github.com/ome/ngff-spec/pull/201)). | ||
|
|
||
| ### Not critical, but suggestions | ||
|
|
||
| - [ ] Clarify when `c` is appropriate. | ||
| - For the moment, I prefer to leave this question open, since the answer | ||
| will be dependent on the use case, and may be refined in future RFCs | ||
| (such as a proposed visualization metadata RFC). | ||
| - [ ] Provide worked OME-Zarr examples for Zeiss CZI and Leica datasets | ||
| (including axis `type` values) and for DTI and frequency-domain (e.g., | ||
| MRI k-space) images. | ||
| - Although these would be nice to have, we consider that this work can | ||
| happen outside of this RFC. | ||
| - [ ] Consider name hygiene for the now unrestricted axis names: NFC-normalized | ||
| Unicode, no leading or trailing whitespace, no `/` or ASCII control | ||
| characters. | ||
| - It turns out that axis names are not currently restricted: they have | ||
| only been limited to tczyx by convention! (See [ome/ngff-spec#180].) | ||
| Therefore, the issue of name sanitisation predates this RFC and should | ||
| be handled separately. | ||
|
|
||
| [ome/ngff-spec#180]: https://github.com/ome/ngff-spec/issues/180 | ||
|
|
||
| ### Additional comments / questions | ||
|
|
||
| - Overlapping labels with an instance axis | ||
|
|
||
| > Is the intent to allow `projectAxis` in that link, or to store such | ||
| masks as a separate multiscales image? Not a blocker. | ||
|
|
||
| The RFC-3 doesn't take a position on this. RFC-8 explicitly allows for | ||
| standalone label images, which renders this question moot. A future RFC may | ||
| eliminate the required minimal set of transformations between labels and | ||
| images. | ||
|
|
||
| - 'omero' 'channels' metadata | ||
|
|
||
| > `omero` describes `channels` as "Array matching the c dimension size" and | ||
| > has `defaultT` and `defaultZ`. With several `channel` or `time` axes it is | ||
| > unclear which axis these refer to. A sentence stating that it applies to | ||
| > datasets with at most one channel and one time axis would suffice. | ||
|
|
||
| Since axes have unique names, this case is already covered. | ||
|
|
||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe also worth mentioning that in zarr v3, there is no fixed relationship between axis order and memory layout. so even recommending a particular axis order would not achieve what the authors want (a consistent memory layout)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that request more informed by coordinate transformations. In many viewer tools, passing an array along with an affine transformation matrix to a viewer will almost certainly require stripping out the channel dimension from the affine matrix. This affine-wrestling (especially when transforming from coordinate systems with channel dimension into a coordinate system without channel dimension) is tricky. Having the channel always be the lead dimension (for instance) would simplify things a bit.
That's just to say the affine wrangling is annoying, but entirely possible and maybe the solution would rather be to not have a coordinate vector concern itself with transforms at all (which would be the way we are discussing over at #580).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then we have very different reads on what the authors were concerned about 😆 I thought they wanted to ensure that the default in-memory layout of decoded chunks was amenable to spatial algorithms like smoothing by packing spatial dimensions close together in decoded memory. But this is not something you can declare in zarr metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to the RFC authors or the review authors? 😅
Also, I trust your statement here but I don't fully grok it. Can you construct me a pathological example with code in which the axis ordering is tczyx but the data is contiguous in t? A concrete example would help the conversation, I think.
Either way, this seems like it is a broader discussion that should not (imho) hold up the RFC. It can always be added later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this gist illustrates part of the point: https://gist.github.com/d-v-b/f53c0e3b6fb1fe2b1314500cc969f0e8
it shows that
zarr, the python lib, can decode the exact same chunk bytes into C or F contiguous memory, depending on a runtime configuration flag. This shows that memory order of decoded chunks is an implementation degree of freedom.and C / F order is just one point in a bigger space of memory layouts that are possible. I would like to support decoding into sparse arrays, which don't have a contiguous memory layout at all.
this can be a hard part of the spec to internalize (I know because it took me a while to have the "a ha" moment), because numpy-style "arrays ARE contiguous sequences of bytes in memory" thinking is so sticky. But for the Zarr format, the memory layout of decoded arrays is outside the spec. Implementations can use C, F, morton, sparse, unordered, whatever. So OME-Zarr can't make any assumptions or statements about the in-memory representation about arrays, unless you want to be stricter than the language in the Zarr spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the authors have a valid concern, but I'd vote for keeping the SHOULD. The wording still allows other ordering, just requires that decision to not be taken without thinking about it. From rfc2119:
I am also hesitant to write SHOULDs in the documentation elsewhere without a clear reference point in the spec