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

Multiscale support #25

Merged
merged 10 commits into from
Sep 7, 2020
Merged

Multiscale support #25

merged 10 commits into from
Sep 7, 2020

Conversation

joshmoore
Copy link
Member

@joshmoore joshmoore commented Aug 19, 2020

  • Use ome_zarr.scale to generate multiscale labels
  • Improve link to source image metadata
  • Drop support for 6D
  • Fix README for --output (fix Update docs README #26)

@joshmoore
Copy link
Member Author

To have this PR generating the multiscale labels, we will need to add a dependency on ome_zarr.

@joshmoore joshmoore mentioned this pull request Aug 31, 2020
@joshmoore joshmoore changed the title Use checked relative path for image link (WIP) Multiscale support Aug 31, 2020
@joshmoore
Copy link
Member Author

@will-moore : this has been to generate the label pyramids. It needs by ome-zarr PR though.

@@ -35,7 +35,7 @@ def image_masks_to_zarr(image: omero.gateway.Image, args: argparse.Namespace) ->

conn = image._conn
roi_service = conn.getRoiService()
result = roi_service.findByImage(image.id, None)
result = roi_service.findByImage(image.id, None, {"omero.group": "-1"})
Copy link
Member

Choose a reason for hiding this comment

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

This probably fails under group sudo but I expect that nobody will notice.

Copy link
Member Author

Choose a reason for hiding this comment

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

So under group sudo one would need to get the group of the image and use that?

Copy link
Member

Choose a reason for hiding this comment

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

If I recall correctly, one's very much locked into the group, so it's more a case of really hoping that the image is in that group. The server could be fixed to better tolerate all-groups context for group sudo so at least there is no exception if the groups do match but I don't think it's a trivial change.

Copy link
Member

Choose a reason for hiding this comment

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

(For other CLI all-groups stuff I was thinking the client could simply not attempt to set that context at all if in group sudo.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. But there haven't been any changes client-side to implement that, right? If so, I assume this will need to be picked up with the rest.

Copy link
Member

Choose a reason for hiding this comment

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

Right. Fine with me to have this as-is while it awaits that.

Copy link
Member Author

Choose a reason for hiding this comment

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

If there's an existing ticket for that issue, please feel free to register this PR with it.


import numpy as np
import omero.clients # noqa
from ome_zarr.data import write_multiscale
Copy link
Member

Choose a reason for hiding this comment

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

In the same spirit as ome/ome-zarr-py#45 (review), we will likely need to define the minimal version of ome_zarr in setup.py as this depends on upstream API additions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I wasn't planning on pushing a release of ome_zarr without color unless there's a pressing need in order to prevent intermediate versions from being created. (Note: this repository is still in need of tests & GitHub actions to detect that dependency.)

@joshmoore
Copy link
Member Author

Since this PR is needed for the other two PRs (ome/ome-zarr-py#45 and ome/omero-ms-zarr#70), merging. I'll open the follow on immediately for review today.

@joshmoore joshmoore merged commit 39367a4 into ome:master Sep 7, 2020
@joshmoore joshmoore deleted the multiscale branch September 7, 2020 06:19
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.

Update docs README
3 participants