-
Notifications
You must be signed in to change notification settings - Fork 62
Rfc5 review response #350
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
Rfc5 review response #350
Conversation
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: John Bogovic <bogovicj@users.noreply.github.com>
Co-Authored-By: Davis Bennett <davis.v.bennett@gmail.com>
| The changes, if approved, shall be translated into bikeshed syntax and added to the ngff repository in a separate PR. | ||
| This PR will then comprise complete json schemas when the RFC enters the SPEC phase (see RFC1). | ||
|
|
||
|
|
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.
before introducing the structure of the particular JSON objects, I would give a high-level overview of the model to orient readers. I think it basically needs to cover these two points:
- images (collections of arrays stored in a multiscale group) are associated with two or more coordinate spaces, which are tagged collections of axis metadata.
- Points are mapped from one coordinate space to another by one or more coordinate transformations.
Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>
…ngff into rfc5-review-response
| The dimensionality of each array coordinate system equals the dimensionality of its corresponding Zarr array. | ||
| Its name is the path to the array in the container, | ||
| its axes have `"type": "array"`, are unitless, and have default names. | ||
| The i-th axis has `"name": "dim_i"` (these are the same default names used by [xarray](https://docs.xarray.dev/en/stable/user-guide/terminology.html)). |
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.
| The i-th axis has `"name": "dim_i"` (these are the same default names used by [xarray](https://docs.xarray.dev/en/stable/user-guide/terminology.html)). | |
| The i-th axis has `"name": "dim_i"`. |
I don't think it's relevant what xarray's defaults are, and also xarray might change their defaults, which would bring this section out of date. Unless xarray compatibility is an explicit design goal we should remove references to xarray from this section (fine to talk about xarray outside the normative section of the spec).
Co-authored-by: Davis Bennett <davis.v.bennett@gmail.com>
…ngff into rfc5-review-response
📌 Closed for review 🚪 - next stepsFirst of all: Thanks for everyone who poured a ton of work to turn this text into what it is right now. I'll refrain from even trying to come up with a complete list of contributors who deserve their mention here. So, why make a cut here and close the debate? What happens next? Closing the debateThe purpose of this PR is, first and foremost, to address the concerns and comments of many reviewers and commenters who took the time to write out their critique, feedback and suggestions for improvements (see rfc5 on ngff page). For me, the time between opening the PR ~two weeks ago and now was a welcome opportunity to expose the text to the community, sharpen it some more, catch a few mistakes before passing it on to the editorial process (see RFC1). Since RFC1 gives reviewers a two-weeks-period to respond to the replies from the author, I think having a static text for them to work on is important. Since it's been about two weeks since this has been open, I think it's fair to have reviewers have their say next. This does not mean that all points raised along the way in this thread have been sufficiently addressed. There are quite a few numbers of issues that came up here that I think SHOULD be fixed - but this isn't the correct forum for it. Please feel encouraged to open these topics as separate issues to give people the chance to interact with the debate on a more visible level and make sure we can spread out the work on these smaller issues on many shoulders. What's next 📈When this is merged, reviewers will have two weeks of time to come to a conclusion about the adoption of rfc5 for the ngff specification text. The working title of the proposal was Moving the spec to a new home 🏡There was a fair amount of discussion whether the current repository structure makes engaging with the progress forward harder, easier, better, worse, etc. Following these voices, a decision was made to explore breaking the spec (specification text, json examples and schemas) to a new place under the umbrella of work on RFC5: https://github.com/ome/ngff-spec, Link to rendered version Some features/perks of the new place:
Once reviewers approved RFC5, the proposal text needs to be translated into a complete addition into the main spec text. #138 contained this work, but we are separating these two efforts here. Hence, this PR is solely to interact with the review process, but work has started on copying everything done here over to the new repo:
Roadmap 🛣️As mentioned plenty times above and in other issues, there are a ton of necessary sanitizing issues that SHOULD go into the spec. At the same time, many people are eagerly waiting for 0.6 to come out or have even started laying out entire implementations. To address both concerns and developments, we envision the following process.
Step 2 is the time to get as many of these sanitizing issues in as possible. I marked a lot of raised issues above as resolved. Not because I want to dismiss rightful criticism, but to make sure these issues I see outside the scope of RFC5 get the attention they deserve in separate break-out issues. It also gives everyone the chance to engage with the discussion so that much of the necessary debate can be resolved until the Step 3: The editorial will at some point close the bar and make sure we have a Step 4: Profit Final wordsThanks for all the contributions, discussions, reviews, comments etc. I haven't been aboard the ngff-train for so long but I'd say it's very much the community that continues to make this project the success it became 🥇 |
Just to be clear, the pathway to raising questions or potentially issues now is opening an issue at https://github.com/ome/ngff? (I ask because in the past when I've done this, I've been told this is not the correct way to engage with RFCs)
Does this mean for an issue to be considered for 0.6dev3, we must also ping either yourself or Josh in addition to opening an issue? If so, what platform would you prefer to be pinged on? |
|
I think in large part, @dstansby, this is about being able to draw a line under things. @jo-mueller is doing that here now for 0.6.dev2 which will go out to review. If you have substantial critiques of 0.6.dev2, then a Comment would be best, which is likely what I had tried to express in the previous case you're referring to. If, however, there's something that can be handled more directly, espeicially if you're already scoping that for the next 0.6 dev version, then by all means, please feel free to use issues. And I'll try to do a better job as described in #138 (comment) of proactively closing issues that crossover from the latter kind to the former kind. It's definitely not required to ping to have your issues eventually considered, but we all have a lot to do, and so at least for me personally, pings & reminders are very welcome if you feel like something hasn't been seen. Thanks! |
joshmoore
left a comment
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.
Thanks, @jo-mueller! 🎉
Found one minor typo that I'll self-commit here post-review. Anything else I find while reading, I'll push directly. Merging so that the link goes live and I can send out to the reviewers.
|
thanks @jo-mueller! What's the recommended way to write a comment for rfc5? Which repo does it belong in? |
|
Comments go in https://github.com/ome/ngff/tree/main/rfc/5/comments. You can use the review template or checkout one of the other files in that directory. |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/ngff-weekly-dev-update-thread/110810/45 |
Hi all,
sorry it took us so long - here come the replies to the reviews and comments to the initial transforms RFC proposal. This PR is supposed to supersede the changes proposed over at #138.
Many thanks to everyone who spent their valuable time digging through the json example data that was provided along with the proposal or went ahead and worked on implementations. The insights gained there helped us a lot to sharpen the text of the spec or make changes where necessary. We hope our changes address the comments and statements made by the reviews.
Of course, thanks goes out to @bogovicj for writing this up to begin with and providing a ton of example data. Also, thanks to @will-moore @dstansby @d-v-b @thewtex for their discussion points along the way (there is likely a substantial number of people I fail to mention here). We hope you find your opinions and inputs reflected in the next version.
I'll save you the details over the changes in this PR - You'll find everything in the review replies and the updated spec document 😉
Caution: 🚧 Minor changes to this PR may still occur in the next few days 🚧
Anticipated FAQs
Answer: We expect future releases to follow a tagged release process over at the ngff-spec repo. Hence, we will open a PR to that target in due time.
Answer: There was recent debate as to what document/resource should be considered authoritative for implementations. There was some consensus that the schemas could be considered as a sort of intermediate implementation. Following this argumentation, introducing schemas for validation will be deferred until the acceptance of the changes to the spec. This is not only to limit effort in writing up the schemas themselves, but also to limit the amount of (unsanctioned) developer versions of schemas out in the wild.
Edit: Relevant for #331 , #326