Skip to content
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
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

virginiascarlett
Copy link
Contributor

@virginiascarlett virginiascarlett commented Mar 7, 2023

Following up on my last PR. Previously, we added a lot more explanation to the 'labels' section of the spec. Now I am not just editing the language, but making some substantive, though minor, changes. Here's a quick summary of what I changed:

  • Added for clarity: "Each label image MUST be stored within a "labels" group."
  • One MUST --> MAY: "the datasets key MUST have the same number of entries (scale levels) as the original" --> "the datasets key MAY have the same number of elements (scale levels) as the original"
  • One SHOULD --> MUST + clearer language: "In addition to the multiscales key, the JSON object in this image-level .zattrs file SHOULD contain another key, image-label, whose value is also a JSON object." --> "The label image .zattrs file MUST also contain the image-label key, whose value is a JSON object."
  • Changed for clarity: "This array contains one JSON object for each unique custom label" --> "This array SHOULD contain one JSON object for each unique custom label."

Thanks!

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2023

Automated Review URLs

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one comment. All the other changes look great!

In addition to the `multiscales` key, the JSON object in this image-level `.zattrs` file SHOULD contain another key, `image-label`,
whose value is also a JSON object. The `image-label` object stores information about the display colors, source image, and optionally,
The label image .zattrs file MUST also contain the `image-label` key, whose value is a JSON object.
The `image-label` object stores information about the display colors, source image, and optionally,
Copy link
Member

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 have image-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?

Copy link
Contributor Author

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?

Copy link
Member

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 the image-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.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Copy link
Member

@will-moore will-moore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks 👍

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/save-a-single-labels-dataset-into-an-ome-zarr/93505/17

@jni
Copy link

jni commented Mar 14, 2024

Added for clarity: "Each label image MUST be stored within a "labels" group."

I would like to point to this imagesc discussion where people are quite unanimous that this is a bad idea — can we remove this line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants