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

Rename labels #64

Merged
merged 3 commits into from
Aug 19, 2020
Merged

Rename labels #64

merged 3 commits into from
Aug 19, 2020

Conversation

joshmoore
Copy link
Member

see: #59

Also renames the single produced array to 0/ to match omero-cli-zarr

volumes:
- "omero:/OMERO"

omeroweb:
Copy link
Member

Choose a reason for hiding this comment

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

You could probably remove this and save the download time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, crud. That wasn't supposed to be included here. I'll push it away, but it's very useful for testing and since it's only downloading once not particularly a problem (unless you mean in travis, and there it's not used at all)

├── .zgroup # The labels group is a container which holds a list
├── .zattrs # of labels to make the objects easily discoverable,
│ # All labels will be listed in `.zattrs` e.g. `{ "labels": [ "original/0" ] }`
│ # Each dimension of the label `(t, c, z, y, x)` should be either the same as the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
│ # Each dimension of the label `(t, c, z, y, x)` should be either the same as the
│ # Each dimension of the label `(t, c, z, y, x)` must be either the same as the

Is there a situation where the dimension would be something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

#66


```
{
"masks": [
"labels": [
"orphaned/0"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"orphaned/0"
"original/0"

Match the example above? Or was this intentionally different to indicate something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we probably need a few examples. Not also that the omero-{cli,ms}-zarr currently use the unimaginative "0" as the default name. To not need to fight with travis again, I'd propose we have a round of "label-naming" after things have settled a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

#67

@@ -763,7 +775,7 @@ private void returnImageMasksDirectory(HttpServerResponse response, List<String>
final ImmutableList.Builder<String> contents = ImmutableList.builder();
contents.add(".zattrs");
if (labeledMasks != null) {
contents.add("labeled/");
contents.add(DEFAULT_LABEL_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

Still needs a terminal "/"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Part of me says we should support both but the removal was unintentional. Thanks.

this.patternForImageMasksDir = Pattern.compile(path + LABEL_GROUP + "/");
this.patternForMaskDir = Pattern.compile(path + LABEL_GROUP + "/(\\d+)/");
this.patternForMaskChunkDir = Pattern.compile(path + LABEL_GROUP + "/(\\d+)/(\\d+([/.]\\d+)*)/");
this.patternForMaskLabeledDir = Pattern.compile(path + LABEL_GROUP + "/" + DEFAULT_LABEL_NAME + "/");
Copy link
Member

Choose a reason for hiding this comment

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

Works okay in this case but among these may want to consider \\Q\\E as in Configuration.getRegexForNetPath if there is any risk of a regex metacharacter someday appearing, e.g., if later allowing these to be configurable. Not a blocker though.

Copy link
Member Author

Choose a reason for hiding this comment

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

#69

@mtbc
Copy link
Member

mtbc commented Aug 19, 2020

Useful discussion on PR but nothing that need block merging.

@mtbc mtbc merged commit 1f1cd7e into ome:master Aug 19, 2020
@joshmoore joshmoore deleted the rename-labels branch August 19, 2020 14:29
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.

3 participants