-
Notifications
You must be signed in to change notification settings - Fork 10
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
Use nested chunk store for labels #82
Conversation
""" | ||
Create an FSStore instance that supports nested storage of chunks. | ||
""" | ||
return FSStore( |
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 assume this logic could be delegated to ome_zarr.format
down the line? This would also make the format version used by this plugin more explicit.
Immediate caveat looking at https://github.com/ome/ome-zarr-py/blob/62b49da4b7200384dac8dec8d9fce48bd727a967/ome_zarr/format.py#L101-L125 is that normalize_keys
would need to configurable (unless it could be set to True
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.
It was set to False
because otherwise path with A/1
was normalised to a/1
: #59 (comment)
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.
That may have been because upstream was doing something wonky. Again, I think that's all settled:
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.
If I want this repo to depend on that fix zarr>=2.8.3
I'd either need to update that in ome-zarr-py (currently "zarr>=2.8.1"
) or add zarr
as a dependency to this repo?
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.
Happy for it to happen in either as long as we eventually unify. fwiw, zarr is now at 2.10.*
So that I understand the compatibility matrix here, is napari effectively only support version 0.2+ of the multiscales specification? |
No, latest napari + napari_ome_zarr still supports e.g.
But before this PR, the labels didn't use nested chunks but the image data did. It looks like napari expects both to be read the same way, so the labels are all black (unless they are also nested). |
NB: Newer versions of zarr should ignore the requested chunk and use the .zarray metadata correctly. |
Cheers, that makes sense although to the best of my knowledge there is no current enforcement that the label image should have the same |
Overall no objection from my side to unifying the writing behavior of this tool so that both images and label images use the same version of the spec. Happy to see this released as |
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.
One major question before we merge this.
@@ -18,3 +20,16 @@ def print_status(t0: int, t: int, count: int, total: int) -> None: | |||
eta = "NA" | |||
status = f"{percent_done:.2f}% done, ETA: {eta}" | |||
print(status, end="\r", flush=True) | |||
|
|||
|
|||
def open_store(name: str) -> FSStore: |
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.
Generally ok with this refactoring, but if it's going public we need to keep ome/ome-zarr-py#110 and similar in mind. i.e. we may not want to control this in the future.
108f8aa
to
52bd467
Compare
In order to fix the napari bug, this PR migrates the EDIT: @will-moore pointed out that the commit associated to #82 (comment) got reverted in #82 (comment). This should be ready for a final round of review |
size_C may not be 1 if the masks have C index set
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/prepping-and-including-roi-masks/48750/24 |
Addition of
|
Built the environment as prescribed in the header. This looks like the workflow 1 in https://docs.google.com/document/d/1LOihtEZWTA7DKsA6zXG1OlQuGTuV7SxAa5OVFSQNeDc/edit#bookmark=id.aygdx8up5vho is tested ? |
@dominikl The
|
@pwalczysko With your test above (#82 (comment)) that is a successful test of ome/napari-ome-zarr#16, so could you give a 👍 on that PR too? Thanks. |
Data is available at https://minio-dev.openmicroscopy.org/idr/v0.3/idr0047/3dc087c/4496763.zarr and can be opened
Turning on the labels: |
NB: In the previous example, the masks on Image https://idr.openmicroscopy.org/webclient/img_detail/4496763/ don't have any Z-index (or T or C) so in IDR they are shown across all Z sections. These dimensions are ignored when exporting labels (see output above
This explains why it is only visible for the first Z-index in |
I also exported https://minio-dev.openmicroscopy.org/idr/v0.3/idr0052/3dc087c/5514374.zarr from idr0052 but mask export fails since the masks overlap: Exception: Mask 1369052 overlaps with existing labels
|
Zoomed-in to see labels: |
|
@pwalczysko Yep, that's what I saw at #82 (comment) |
|
In summary,
|
The strategy for It would be nice if there was some way of getting |
@pwalczysko I remembered that there is a way of handling overlapping masks as described at https://github.com/ome/omero-cli-zarr#masks (using an idr0052 image as an example).
|
For me, the workflow described in https://github.com/ome/omero-cli-zarr#masks for overlapping masks ended with error
|
@pwalczysko My output for that was:
The first line to differ is where you have |
Sorry, my
Also, it opens and looks fine in napari too. |
@will-moore https://pypi.org/project/ome-zarr/0.2.0/ has just been released with the new |
This uses nested chunks when exporting labels from OMERO.
Without this, the Image data is nested but labels are not - so they are ignored in napari.
Also, it updates labels to
v0.3
(corresponding to #70 for image data) where wediscard any 'empty' dimensions. This means that the image data and labels data have same number of dimensions so that they display correctly in napari.
Testing setup
You'll need a python environment with:
omero-cli-zarr
,ome-zarr-py
,napari-ome-zarr
installed.If we just want the released versions of everything:
Create an environment, using this
environment.yml
:To get a specific branch of https://github.com/ome/omero-cli-zarr or https://github.com/ome/ome-zarr-py or https://github.com/ome/napari-ome-zarr:
pip uninstall omero-cli-zarr
# orome-zarr
ornapari-ome-zarr
pip install -e .
# reinstall from the current branchTo test with https://idr.openmicroscopy.org/webclient/?show=image-6001247:
With
ome-zarr-py
from ome/ome-zarr-py#114 needed for down-sampling of labels with fewer than 5D.The shape of the labels should match the image data (same number of dimensions) although size_c for channels will be 1.
This also needs ome/napari-ome-zarr#16 for viewing: