-
Notifications
You must be signed in to change notification settings - Fork 42
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
bioformats2raw.layout #112
Conversation
Pushed some schema fun, @will-moore, if you want to take a look. I need to force push away the commits from #110 but I'll hold off on that a bit. |
I think the latest commit is missing |
Thanks. Pushed.
Compared to the debate? |
Now that #110 is merged, can you merge |
Done, @will-moore |
Pushed a statement about the recent realization that we want this layout for previous versions as opposed to upcoming versions. There might be a function in bikeshed along the lines of "applicable-to-version" that we may want to make use of. |
Likely a last call for comments here. I'm going to move on to the various PRs:
Note: https://github.com/ome/ome-zarr-metadata will not be used for this release. |
While trying to get the `bioformats2raw.layout` specification (ome/ngff#112) this raised its head again. Following the discussion in ome/ome-model#158 changing the size checks to `40` passes. fix: ome/bioformats#3810 fix: ome/napari-ome-zarr#47 (comment)
* Update Hex40 definition While trying to get the `bioformats2raw.layout` specification (ome/ngff#112) this raised its head again. Following the discussion in ome/ome-model#158 changing the size checks to `40` passes. fix: ome/bioformats#3810 fix: ome/napari-ome-zarr#47 (comment) * Bump to min_length=40
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.
A few comments. I think the PR includes the bulk of the rules describing the bioformats2raw
layout and its associated metadata. Is the proposal still to port this transitional metadata specification only to the current stable 0.4 specification?
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.
Noticed a couple of small typos while reading latest comments from @sbesson.
Co-authored-by: Melissa Linkert <[email protected]> Co-authored-by: Sébastien Besson <[email protected]>
Yes. I'll do once all edits are made (Hopefully this morning). |
From my POV, I think we are ready to move forward with this PR as well as the bioformats2raw PR itself. Python I think will follow relatively quickly and then we'll look into Javascript. @tischi: do you think there is interest in trying to add this to MoBIE now? or wait for the non-transitional version? |
I would be interested adding this to the |
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.
Apart from one small clarification, I think this is fine to merge.
latest/index.bs
Outdated
- If "OME/METADATA.ome.xml" or the "series" attribute do not exist: | ||
- existing "plate" metadata will take precedence if it exists, or |
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.
Does will
here actually mean MUST
?
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.
Hmmm.... I think that's right, but what I was trying to get across is that "the next statement only holds when "plate"
isn't defined". Is that the same thing as the MUST?
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.
or more simply:
- If "OME/METADATA.ome.xml" or the "series" attribute do not exist and "plate" metadata is not defined
?
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 what bothers me in the logic is that the "plate MUST take precedence" applies much higher, e.g. even if the XML exists. Let me try to break all the clauses apart.
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.
Pushed this proposal for feedback, @melissalinkert:
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.
Primary question from my side is if it's actually:
Matching "series" metadata (as described next) SHOULD be provided ...
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.
Latest version is definitely clearer, thank you. I think Matching "series" metadata (as described next) SHOULD be provided
makes sense, but I don't have strong objections to MAY
.
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.
If we're unsure, I'll err on the side of the stricter SHOULD
and then we can downgrade it later.
bioformats2raw.layout SHA: 449fbca Reason: push, by @joshmoore Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/intermission-ome-ngff-0-4-1-bioformats2raw-0-5-0-et-al/72214/1 |
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/using-bioformats2raw-for-creating-ome-zarr-scale-format-string/72716/6 |
First update for supporting bioformats2raw output as described in #104. Introduces the concept of "transitional" metadata to allow readers to start supporting extensions that have grown in the community before they are added here.
Draft previwable under: http://api.csswg.org/bikeshed/?url=https://raw.githubusercontent.com/joshmoore/ngff/bf2raw/latest/index.bs#bf2raw
Note: currently builds on top of #110 to prevent conflicts.