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 labeled images #45

Merged
merged 39 commits into from
Sep 4, 2020
Merged

Multiscale labeled images #45

merged 39 commits into from
Sep 4, 2020

Conversation

joshmoore
Copy link
Member

@joshmoore joshmoore commented Aug 19, 2020

ome-zarr now expects a multiscale zgroup rather than a single zarray as the content of a label. Along with the previous round of changes (#44 (comment)) this means to modify an existing ome-zarr with masks you need to:

  • rename masks/ to labels/
  • run ome_zarr scale 6001240.py/labels/{0,1}
  • merge the metadata from labels/0/.zattrs into labels/1/.zattrs
  • add 1 to the labels/.zattrs "names" element, optionally removing 0

(This will be put into a script to be run against s3.embassy.ebi.ac.uk)

Interestingly, with the current changes, loading is still successful without the changes above since the the mask is treated as a raw zarr array.

Summary of other user changes:

  • It's now possible to pass the labels/ directory to napari. The mask is opened visible, and the image is loaded invisible. This is the first usage of the link {"image": { "array" .... See Fix masks.[].image.array to match spec omero-cli-zarr#19 . Future work will likely make it possible to point at the label itself and have the images loaded, too.
  • New ome_zarr create command for creating sample data. Note: colors are not working correctly yet.
  • New ome_zarr scale command
  • ome_zarr info now displays different output.
  • ome_zarr download now downloads recursively.

Summary of other developer changes:

  • Split main script into napari.py, cli.py, utils.py, reader.py (beginning of refactoring for code reuse; multiple napari plugins may also be a good path forward).
  • Splitting of BaseZarr into BaseZarrLocation and Layer/Spec types (significantly simplifies different starting points)
  • Enforce mypy in all method definitions (aided with refactoring).

ome_zarr/reader.py Outdated Show resolved Hide resolved
The introduction of multiple types per node of the Zarr hierarchy
(e.g. multiscales AND labeled image) made the existing code flow
difficult to extend and test. This reworks the previous BaseZarr
type into multiple hierarches:

 - ome_zarr.io: Locations hierarchy for choosing Remote or Local
 - ome_zarr.reader: Spec hierarchy for choosing metadata types

The general code flow consists of:

 - ome_zarr.napari.napari_get_reader()
   - ome_zarr.io.parse_url()
   - ome_zarr.reader.Reader.__call__()
     - ome_zarr.reader.Layer()
       -ome_zarr.reader.Spec() [recursion]
   - ome_zarr.napari.transform
 - replace test lookup for labels/coins
 - don't write labels group if no label exists
Provides the `ome_zarr scale` CLI command as well as the
`ome_zarr.scale.Scaler` class for downsampling arrays as
multiscales.

originally: ome/omero-ms-zarr#61
New tests and code require a substantial number
of requirements like opencv and Qt. Rather than
try to encapsulate that configuration directly
in .travis, we copy the napari-omero strategy
of using conda in GH actions.
ome_zarr/data.py Outdated
if pyramid[0].shape[CHANNEL_DIMENSION] == 1:
image_data = {
"channels": [{"window": {"start": 0, "end": 1}}],
"rdefs": {"model": "grayscale"},
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 OMERO metadata JSON I think we previously had "greyscale" matching RenderingModel. I don't know if this matters or what it should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to defer to @will-moore, but I agree at least the three zarr related repos should use uniform spelling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup. OMERO has greyscale everywhere. napari grayscale. I'll update to use the former and if we need to, we can transform in ome_zarr.napari.

NB: ome-model primarily uses "e", "a" appears only twice in docs. Bio-Formats leans more towards "a" including the CLI argument.

ome_zarr/io.py Show resolved Hide resolved
ome_zarr/reader.py Outdated Show resolved Hide resolved
Co-authored-by: Mark Carroll <[email protected]>
@will-moore
Copy link
Member

Testing as above with latest commits from here and omero-cli-zarr is working:

$ omero zarr export Image:139615
$ omero zarr masks Image:139615
$ napari 139615.zarr

Screenshot 2020-09-02 at 13 26 10

I noticed the labels layer is visible initially. Is that a change of behaviour?
Also, I wonder if we want to rename masks -> labels for:

$ omero zarr masks Image:139615

?

@will-moore
Copy link
Member

I tried downloading:

$ ome_zarr download https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/6001240.zarr/
WARNING:ome_zarr.io:unreachable: https:/s3.embassy.ebi.ac.uk/idr/zarr/v0.1/6001240.zarr/labels/.zarray -- details logged at debug
WARNING:ome_zarr.io:unreachable: https:/s3.embassy.ebi.ac.uk/idr/zarr/v0.1/6001240.zarr/labels/.zgroup -- details logged at debug
ERROR:ome_zarr.cli:. already exists!

I guess the s3 data hasn't been updated from masks -> labels yet, but even if labels are missing, should the image data itself still download OK?
e.g. When I try napari 'https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/6001240.zarr/' even though there are no labels, this opens the image fine.

@joshmoore
Copy link
Member Author

I noticed the labels layer is visible initially. Is that a change of behaviour?

Yeah, I noticed that as well. One of my bug fixes must have reverted. I assume it's not overly critical, but I'll look into it.

I tried downloading ... I guess the s3 data hasn't been updated from masks -> labels yet

Definitely. omero zarr masks is the only source of test data at the moment. Once these PRs go in, I can update the public data. Or we hold off for the color changes.

@will-moore
Copy link
Member

My point was not so much that the 'masks -> labels` updates need to happen on s3, but that any image fails to download if it doesn't have any labels.
Or, maybe it's not the lack of labels that is breaking download, but something else.
Trying another ID from https://downloads.openmicroscopy.org/presentations/2020/Dundee/Workshops/NGFF/zarr_diagram/ which I don't think has any masks/labels

$ ome_zarr download https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/179706.zarr/
WARNING:ome_zarr.io:unreachable: https:/s3.embassy.ebi.ac.uk/idr/zarr/v0.1/179706.zarr/labels/.zarray -- details logged at debug
WARNING:ome_zarr.io:unreachable: https:/s3.embassy.ebi.ac.uk/idr/zarr/v0.1/179706.zarr/labels/.zgroup -- details logged at debug
ERROR:ome_zarr.cli:. already exists!

@will-moore
Copy link
Member

Ah, this was a lack of output dir.
This works:

$ ome_zarr download https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/179706.zarr/ --output test

ome_zarr/reader.py Outdated Show resolved Hide resolved
node which will likely be turned into a lower layer for display.

By unsetting visible, the node (and in turn the layer) can be
deactivated for initial display. By default, the value of the current
Copy link
Member

Choose a reason for hiding this comment

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

Slightly confused: "by default" suggests a means of override but I am not seeing what that is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing a value would override the None. I'm pushing a commit to rework the wording. Let me know if you think it's better.

@will-moore
Copy link
Member

The labels layer is not visible when opened in napari now. 👍

For $ ome_zarr download https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/179706.zarr/, should --output be a required parameter (since it will fail with the default output dir of .)?

Copy link
Member

@sbesson sbesson left a comment

Choose a reason for hiding this comment

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

👍 for the overall refactoring of this class into more tractable and testable modules. Are we effectively approaching an 0.1.0?

self.pre_nodes: List[Node] = []
self.post_nodes: List[Node] = []

# TODO: this should be some form of plugin infra over subclasses
Copy link
Member

Choose a reason for hiding this comment

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

I assume another todo might be to add some validation for incompatible specs i.e. I assume a node cannot be both a Label and Labels

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 the Labels pointed to itself with . it might just work...

ome_zarr/data.py Show resolved Hide resolved
ome_zarr/data.py Show resolved Hide resolved
@joshmoore
Copy link
Member Author

should --output be a required parameter (since it will fail with the default output dir of .)?

Ah thanks. I've fixed this to work again without the flag.

@joshmoore
Copy link
Member Author

Non-Windows tasks are all now passing. It's going to be fiddly to get Windows passing, so I'd like to do that in a new PR.

Any objections to merging?

@mtbc
Copy link
Member

mtbc commented Sep 4, 2020

GFI. I think any fixes will be much smaller increments than this.

@joshmoore joshmoore merged commit 8199ed6 into ome:master Sep 4, 2020
@joshmoore joshmoore deleted the multiscale branch September 4, 2020 10:17
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.

4 participants