-
Notifications
You must be signed in to change notification settings - Fork 18
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
feat(ngff): Support displaying "labels" for "multiscales" nodes #242
base: main
Are you sure you want to change the base?
Conversation
@manzt That's looking great, thanks. I also tried testing with a different IDR sample at https://ome.github.io/ome-ngff-validator/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0052A/5514375.zarr. |
Agree this would be ideal. From an image root, where can one find the paths to the associated labels? I was only about to find paths in the one metadata pointing from labels up. |
You just have to check for the existence of |
Ok, I updated the PR to handle this case. Now if you provide a labels node, it will just open as a regular multiscale array. But we will look for |
@manzt Nice - that's loading both labels now 👍 . I think there's an issue with the T-slider not updating labels? When I scroll through T I don't see any change in labels (but they do update when I scroll through Z). I tried to check that the labels really do update through T by opening the label image itself... This works in vanilla vizarr: https://hms-dbmi.github.io/vizarr/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0052A/5514375.zarr/labels/Chromosomes |
Not totally sure. I can have a look. |
This is awesome to see @manzt ! ❤️ cc @tcompa @zonia3000 Do you have capacity to help here @zonia3000 ? Would be great to try this on our typical test data (e.g. this one with labels in both 2d & 3d images: https://zenodo.org/records/13305316). And then to see if we can help with some of the things mentioned as TODOs in this PR :) |
Sounds good! First thing would be to see if you can get your own images (with labels) displayed with the current state of the PR. Then, with something visible we could try to iron out exactly what we need to prioritize to make it usable/useful, working backwards what code to write. For example, I know that we will at least need to fine a way to make Some other ideas:
We can then focus on making the rendering ( |
26cb7f5
to
798405f
Compare
Hi there, thanks a lot. As a first comment, I tried looking at one of the Fractal images - which is available at https://raw.githubusercontent.com/tcompa/hosting-ome-zarr-on-github/refs/heads/main/20200812-CardiomyocyteDifferentiation14-Cycle1_mip.zarr/B/03/0/ Sanity check 1: this image is viewed correctly in the standard vizarr viewer: Sanity check 2: the image has a label array (under Minor glitch in this page: I can add the same channel N times: However something seems a bit off when I load the image+labels: The My first guess for the explanation is that our label is a multiscale array. The logs include a 404 response for the GET of https://raw.githubusercontent.com/tcompa/hosting-ome-zarr-on-github/refs/heads/main/20200812-CardiomyocyteDifferentiation14-Cycle1_mip.zarr/B/03/0/labels/nuclei/.zarray, but in fact there is no array at that path. There would be one at https://raw.githubusercontent.com/tcompa/hosting-ome-zarr-on-github/refs/heads/main/20200812-CardiomyocyteDifferentiation14-Cycle1_mip.zarr/B/03/0/labels/nuclei/0/.zarray instead (note the additional |
In the last image + labels case, I'm seeing console errors |
Yes, these are the typical dimensions we'd have. Context:
My questions from this: |
we can support labels of different dimensions with the caveat that the axis labels MUST be a subset of the source image. Otherwise there is no way to align the selections across layers. |
@jluethi No, labels shouldn't need to have a C axis, although I see in the examples above we do have |
That makes sense! And yes, I would agree that the axis of labels MUST be a subset. In our case, it's often czyx images & zyx labels. There is the weird edge-case of arrays that have singleton dimensions. I think the limitation is fair that labels cannot introduce new singletons (e.g. a label shouldn't be saved as tzyx with singleton t dimension if the input image was czyx). |
Great, thanks everyone! I forgot about the |
9cba886
to
87a665d
Compare
Not entirely sure what to do about this one. The labels have a z axis of length 1, while the source image has a z axis of length 25.
we have logic to try to "align" selections across layers, but I'm not sure what the behavior would be here. I guess we could inspect the shape and ignore selections from dimensions that aren't exactly aligned. |
@manzt I wouldn't worry about trying to align labels for https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0047A/4496763.zarr/ - I think that is clearly invalid. |
I actually was able to support this by generalizing the idea of special-casing the ![]() |
Was also trivial to support, although invalid. We can just treat empty |
Ok, so the implementation of the dynamic LUTs I made fails when the table is really large due to an issue with WebGL uniform sizes. I've updated the implementation to fallback to the default random LUT (and log a warning) if we can't use the colors defined in the OME-NGFF metadata. I don't think that should be a blocker for this PR, and we can follow up separately to improve the shaders (i.e., sample from a texture rather than use uniforms). All the links you shared @will-moore seem to be working now.
One thing to note is that this one is very slow due to how much data we need to load because of the 3D chunks. |
Wow, really nice work. Thanks @manzt! They're looking great! |
I'm not sure when I'll be able to push the additional UI changes through. However, I think in this state I'd vote to try to get this merged (maybe behind an Then, we have separate, smaller PRs to clean up the UI for layer properties ( The main outstanding issue IMO is that merging "as is" changes the default behavior of vizarr for older URLs, so I'm trying to think of how we could keep the previous behavior and maybe op-in to showing the labels? Does that make sense? We could maybe show that the image has labels in the UI, but you need to toggle them on first? |
Fully agreed on having one default colormap and not exposing this further for this effort here! I'd have a strong preference for the "large palette" one, because the smaller one repeats so often that neighboring labels with unique values often get the same color assigned in the example here.
That would work for us!
That would be even better! :) We can either directly start with this or start with the experimental flag and then add this. I think having them toggled off by default is fine and one can then activate them as wanted.
We'd be happy to make contributions for this! :) |
Alright, let's just go with that one for now and we can reassess in the future if/when something comes up.
Ok, I'll push this PR to get some indicator of labels ("off" by default), which can be enabled (as a group) with opacity sliders. We could then add more granular controls to toggle on/off individual labels in a separate PR. Will try to land by the end of the week, but hopefully the deploy preview URLs are usable for the time being to test out the feature futher. |
That's awesome! We've deployed the branch locally as well to use it with some non-public data and our authentication, I'll have a look if I spot any general issues coming up. Very much looking forward to using this more, thanks a lot for your efforts here Trevor! |
Towards #45
demo: https://deploy-preview-242--vizarr.netlify.app/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.4/idr0052A/5514375.zarr
Making progress—still a work in progress, but the initial plumbing is in place. That will need a refactor later alongside broader state management (probably at some latter point). However, things are showing up.
Key files:
src/layers/LabelLayer.ts
– Custom deck.gl layer for rendering labels.src/components/LayerController/Labels.tsx
– UI controls forImageLabelLayer
, including the opacity slider.With this setup, we can iterate on rendering and UI controls. This PR is in a good spot to move forward.
Screen.Recording.2025-02-25.at.23.31.28.mov
@will-moore @jluethi @joshmoore