generated from w3c-ccg/markdown-to-spec
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Resolving misc issues and ambiguities in labels section #170
Open
virginiascarlett
wants to merge
2
commits into
ome:main
Choose a base branch
from
virginiascarlett:labels_minor_fixes
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
2 commits
Select commit
Hold shift + click to select a range
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 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
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.
Since there are no mandatory contents of the
image-label
dictionary, it would be valid to haveimage-label: {}
.It seems that making the spec more strict (a breaking change) to require the
image-label
dict is probably not worth it in that case?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 that that 'MUST' was in response to @sbesson's comment on my previous PR:
"For the image-label specification, my personal opinion would be to enforce it at a MUST level. Doing so would have the advantage of making it unambiguous and potentially reducing the number of graph operations."
Maybe @sbesson can comment here?
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 for the ping @virginiascarlett. Trying to justify, the rationale behind my original statement, I see a label image as a specialized type of multiscales image. At the moment, the specification enforces that such data must be stored within a well-defined
labels
hierarchy but moving forward, I could certainly imagine a relaxation of this constraint.A typical use case that comes immediately to mind is the one where segmentation / classification is performed against a read-only Zarr dataset e.g. public data and the output of this process needs to be stored as a new dataset. At the moment, the structure which is the most compliant with the spirit of the specification is create an artificial
labels/<label_name>/
hierarchy under the root even if there is no multiscales image. Assuming we relaxed this constraint to allow label images to be stored at the root of the Zarr dataset, I would argue theimage-label
metadata would become a critical element to identify what we are dealing with.That being said, Will's point makes sense and if this specification change simply leads to most implementations adding
image-label: {}
in the metadata, I don't find this is particularly helpful.Happy to separate this discussion from your proposal, revert the requirement level back to the SHOULD level and come back to this discussion if we decide to specify standalone label images.
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 would be happy with keeping
image-label
at the SHOULD level if we agree.Good points raised by @sbesson on having standalone label images. I agree those would be a nice addition, and worth discussing in its own issue. I may start one now, but will likely neglect it until the next release is pushed through.
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.
Done!