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

Will moore/ome zarr #816

Closed
wants to merge 9 commits into from

Conversation

will-moore
Copy link
Contributor

@will-moore will-moore commented Dec 23, 2020

See #815

This is initial work towards supporting OME-Zarr images.

Currently, I'm using a config file at https://gist.githubusercontent.com/will-moore/b48704fa803ad6ab977214c5e7cace24/raw/b4a317d19ebd45081ae0c512d4907b0d9dc57aca/omezarr.json
and this uses the new OmeZarrLoader to load a Zarr (https://hms-dbmi.github.io/vizarr/?source=https://s3.embassy.ebi.ac.uk/idr/zarr/v0.1/179706.zarr). Quite a lot of this is hard-coded for now: Only showing a single Z and single T dimensions.

To get this working, I've just copied some code from vizarr instead of using the viv createZarrLoader() which expects a consolidated .zmetadata file, which doesn't exist for OME-Zarr.
I don't know if we want to have Vitessce depend on vizarr. Maybe see how much code is duplicated once I've cleaned this up a bit.

I'll try to reduce some of the hard-coding and try to get my head around more of this as I go...

Screenshot 2020-12-23 at 23 20 39

@keller-mark
Copy link
Member

Thanks, I just made a new PR into your branch with some comments here will-moore#1

I am not super familiar with the OME-Zarr format but maybe @ilan-gold or @manzt can also take a look after the holiday break

@keller-mark
Copy link
Member

keller-mark commented Dec 24, 2020

As far as moving this type of code into viv or vizarr, there was some discussion here hms-dbmi/viv#290 and I think the conclusion is that making the Viv loader instances is fairly application-specific and should be easy to implement, so the implementation started in this PR is perfectly fine

@ilan-gold
Copy link
Collaborator

ilan-gold commented Dec 24, 2020

@will-moore In terms of addressing hardcoding/props for rendering, the layers attribute of the returned object from the loader should follow this schema, I believe, if you wish to override these sorts of things (channel settings, selections etc):

"rasterLayer": {
"description": "The properties of this object are the rendering settings for the raster layer.",
"additionalProperties": false,
"required": ["channels", "colormap", "index", "opacity", "type"],
"properties": {
"channels": {
"type": "array",
"items": {
"type": "object",
"additionalProperties": false,
"required": ["selection"],
"properties": {
"color": {
"type": "array",
"items": { "type": "number" },
"description": "The color to use when rendering this channel under the null colormap."
},
"selection": {
"type": "object",
"description": "Determines the channel selection, e.g. some Z and time slice."
},
"slider": {
"type": "array",
"items": { "type": "number" },
"description": "Determines the range for color mapping."
},
"visible": {
"type": "boolean",
"description": "Determines whether this channel of the layer will be rendered in the spatial component."
}
}
}
},
"colormap": {
"oneOf": [
{
"type": "string",
"description": "The name of the colormap to use for this layer."
},
{
"type": "null",
"description": "Use the solid color definitions."
}
]
},
"index": {
"type": "number",
"description": "The index of the layer among the array of layers available in the image file."
},
"opacity": {
"type": "number"
},
"domainType": {
"type": "string",
"enum": ["Full", "Min/Max"],
"description": "Determines the extent of the channel slider input element in the layer controller."
},
"type": {
"type": "string",
"enum": ["raster"]
}
}
},

Then you likely have to alter this function (or the ones it calls) in order to handle these pre-initialized render/channel settings coming from the loader (in contrast, currently the application gets these settings via a pre-configured view config like this or is auto-inferred using our stats functions for rendering/best guess for channel selections, not the source itself):
export function useRasterData(loaders, dataset, setItemIsReady, addUrl, isRequired, onLoad = null) {
const [raster, setRaster] = useState();
// Since we want the image layer / channel definitions to come from the
// coordination space stored as JSON in the view config,
// we need to set up a separate state variable here to store the
// non-JSON objects, such as layer loader instances.
const [imageLayerLoaders, setImageLayerLoaders] = useState([]);
const [imageLayerMeta, setImageLayerMeta] = useState([]);
const setWarning = useSetWarning();
useEffect(() => {
if (!loaders[dataset]) {
return;
}
if (loaders[dataset].loaders.raster) {
loaders[dataset].loaders.raster.load().catch(e => warn(e, setWarning)).then((payload) => {
if (!payload) return;
const { data, urls } = payload;
setRaster(data);
urls.forEach(([url, name]) => {
addUrl(url, name);
});
const { layers: rasterLayers, renderLayers: rasterRenderLayers } = data;
initializeRasterLayersAndChannels(rasterLayers, rasterRenderLayers)
.then(([autoImageLayers, nextImageLoaders, nextImageMeta]) => {
setImageLayerLoaders(nextImageLoaders);
setImageLayerMeta(nextImageMeta);
if (onLoad) {
onLoad(autoImageLayers);
}
setItemIsReady('raster');
});
});
} else {
// There was no raster loader for this dataset,
// and raster should be optional.
setImageLayerLoaders([]);
setImageLayerMeta([]);
if (isRequired) {
warn(new LoaderNotFoundError(dataset, 'raster', null, null), setWarning);
} else {
setItemIsReady('raster');
}
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [loaders, dataset]);
return [raster, imageLayerLoaders, imageLayerMeta];
}

Note that this function calls initializeRasterLayersAndChannels which I believe does auto-initalization so removing/altering that behavior is an essential feature here, I think. One option is probably to use initializeLayerChannelsIfMissing instead but I could be wrong. I'll try to look into this more in the coming days to be certain, but wanted to give some suggestions in case you feel inclined to start poking around.

The overarching machinery to handle the rendering settings being set from this location instead of a view configuration like the one you have in your gist should be in place otherwise. Sorry to put you in the deep end here in terms of developing in this repo since this is basically a new feature, but hopefully it's not too painful - we're always here to answer questions too or help out with the code when we can!

I think @manzt is best positioned to discuss zarr code duplication between viv/vizarr/vitessce. I also don't feel sure about depending on vizarr, but it's probably better than duplicating the code.

@keller-mark
Copy link
Member

Copying the demos from the comments of the will-moore#1 PR:

Demo 1

Demo 2

@ilan-gold ilan-gold mentioned this pull request Dec 27, 2020
@ilan-gold
Copy link
Collaborator

@manzt @will-moore Are you all aware of something with the padding on the tiles? It seems like they are padded, but the size of the zarr store does not report that. Am I missing something?

@manzt
Copy link
Member

manzt commented Dec 29, 2020

Sorry just catching up.

I think @manzt is best positioned to discuss zarr code duplication between viv/vizarr/vitessce. I also don't feel sure about depending on vizarr, but it's probably better than duplicating the code.

I have some thoughts, but first it would be useful to define each of our tools:

Name Description Primary export
Viv JavaScript rendering library (build on deck.gl + zarr.js + geotiff.js) Loaders + Layers
Vitessce Web application / component library (built on React + Deck.gl + Viv + PubSub) Vitessce components
Vizarr Web application (built on React + Recoil + Viv) not a library

We can think of Viv as a prop-driven toolkit for rendering biomedical images. However, this means that applications using Viv need to decide on how to manage the state of those props (e.g. loaderSelection, colorValues, sliderValues, etc..). You can use Deck.gl (and subsequently Viv) outside of a JS framework entirely, and have a static site (with not UI) rendering an image with hard-coded props.

I have opened an issue in Vizarr with a more detailed description, but I think the best approach to reduce duplicated code is to extract the ome-specific, creating zarr-arrays and parsing metadata, to a shared module. Vizarr can just use this package to derive it's initial state (like napari), and vitessce can translate that initial state into it's own (which is a subset of vizarr's).

@manzt @will-moore Are you all aware of something with the padding on the tiles? It seems like they are padded, but the size of the zarr store does not report that. Am I missing something?

Could you be any more specific? URL to a data source would be useful. What do you mean by padding? screenshot?

@ilan-gold
Copy link
Collaborator

Could you be any more specific? URL to a data source would be useful. What do you mean by padding? screenshot?

There are a few demos linked above that I have checked out locally and noticed this odd behavior. I am not sure what is going on, but the returned tiles appear to be uniformly 1024 x 1024 at the highest-resolution zoom level, which is unexpected (I think) given that they should be something else on the border of the image. That is, I was under the impression there was no padding in zarr stores, and if there was it would be reported in the height/width which it is not here if I am noticing this right (top 2 are dimensions, bottom 2 are tiles):

Screen Shot 2020-12-29 at 12 39 47 PM

@manzt
Copy link
Member

manzt commented Dec 29, 2020

That is, I was under the impression there was no padding in zarr stores,

Every zarr chunk (tile) is the same shape, even if it is an edge chunk/tile. So yes, all chunks are "padded".

@ilan-gold
Copy link
Collaborator

ilan-gold commented Dec 29, 2020

Ok @manzt there must be something going on with Viv then. I'll extract the OME loading logic to there temporarily so I can debug. I thought this would have been handled by hms-dbmi/viv#308 but I must be wrong or missing something, as I often am :)

@manzt
Copy link
Member

manzt commented Dec 29, 2020

I thought this would have been handled by hms-dbmi/viv#308 but I must be wrong or missing something, as I often am :)

The effect is just that Zarr tiles are always the same size, regardless of whether the dimensions of the base image are a multiple of the tile size. I don't think this should be an issue because it's ok that getRasterSize doesn't match (each tile is rendered independently and defines it's own shape).

@keller-mark
Copy link
Member

Just so I can learn more about the OME-Zarr metadata format - besides the code duplication and lack of usage of the rdefs default T and Z values, is there anything technically wrong with the PR here? I tried to loosely follow the vizarr code, but had some questions:

  • are the dimensions of an OME-Zarr always t, c, z, y, x? It looked like on this line the number of dimensions may vary between 2 and 5? Whereas here all 5 are currently hard-coded
  • is an OME-Zarr necesssarily pyramidal?

@manzt
Copy link
Member

manzt commented Dec 30, 2020

are the dimensions of an OME-Zarr always t, c, z, y, x? It looked like on this line the number of dimensions may vary between 2 and 5? Whereas here all 5 are currently hard-coded.

Yes, dimensions are always t.c.z.y.x for OME-Zarr. Sorry that bit in createLoader is confusing. In vizarr, the loaderSelection is of type number[]. The loader only needs to know which dimensions are x and y in order to index correctly. This is because dimensions might not have labels for generic zarr, but we assume the last two are always y and x. I've moved away from keeping "dimensions" on the zarr loaders mostly to separate the data (lazy-arrays) from the optional metadata that is used in the UI to create selections (but not important to Viv at all).

is an OME-Zarr necesssarily pyramidal?

OME-Zarr describes a hierarchical layout for many related images. I believe each OME-Zarr image implements the multiscales and omero specification (pyramidal + rendering metadata).

This might be useful for reference: https://ngff.openmicroscopy.org/latest/#image-layout

"Each image is a Zarr group, or a folder, of other groups and arrays. Group level attributes are stored in the .zattrs file include "multiscales" and "omero" below."

So as a client the trick is to read the .zattrs for the zarr group (provided via url), and then determine what type of node it is (multiscale, plate, etc).

is there anything technically wrong with the PR here?

I don't believe so, and I think this a step in the right direction. I just wanted to offer some options for greater code reuse.

@will-moore
Copy link
Contributor Author

Hi, thanks for all the discussion above. Just catching up...

As far as this PR is concerned, I'd like to get the OmeZarrLoader to provide the rendering settings from the OME-Zarr file (if available) instead of using the defaults (as in Demo 1) or providing them in the url/config (as in Demo 2).

But as mentioned above, adding this to the image layer I'm returning from the loader like this:

    const image = {
      name,
      type: 'ome-zarr',
      url: this.url,
      channels: [
        {
          selection: { channel: 0, time: 0, z: 0 },
          color: [255, 0, 0],
          slider: [167, 2222],
          visible: true,
        },
        {
          selection: { channel: 0, time: 0, z: 0 },
          color: [0, 255, 0],
          slider: [288, 4095],
          visible: true,
        },
      ],

has no effect, presumably because it's being overwritten or just ignored.
But my limited digging into this so far hasn't given me an idea how I'd fix that. Haven't looked at all the hooks enough to grok where the default values are coming from etc.

I haven't discussed any of this with the OME team yet cc @joshmoore - This was an idea I thought worth trying over the holidays but we're all back on Tuesday so will hopefully be able to give it a bit more focus then.

@manzt Thanks for creating hms-dbmi/vizarr#69 - will give it some thought...

@keller-mark
Copy link
Member

I'd like to get the OmeZarrLoader to provide the rendering settings from the OME-Zarr file

Currently, we have these two pretty disconnected notions of:

  • loaders: classes that are mostly concerned with fetching data through a .load() method
  • coordination space: the part of the view config that stores the visualization parameters

This is an important feature that we should discuss more generally (i.e. how to initialize things in the coordination space upon loading a file).

We currently have round-about ways to initialize visualization parameters on initial load if they are missing:

Probably not a coincidence that the code to do these initializations is some of the most confusing in the whole repo:
requiring useEffects and odd callbacks to the use___Data hooks with associated utility functions

I think instead, we could allow the .load() method of the loaders to return two things:

  • data
  • initial values for any coordination type, which will be used if the current value of a coordination scope is null. The use___Data hooks can take the coordination value setter functions as an additional argument, which can hopefully replace the onLoad callbacks that we currently have

This should also have the benefit of fixing another bug: currently, the initializations are dependent on having certain components in the current layout (e.g. cell set colors cannot be initialized unless the cellSets component is present)

@joshmoore
Copy link

#816 (comment) Yes, dimensions are always t.c.z.y.x for OME-Zarr.

For the moment. A loosening of the ordering is certainly in the works. Then it will be necessary to read metadata about the order in order to interpret the possibly-non-5D array appropriately.

#816 (comment) I believe each OME-Zarr image implements the multiscales and omero specification (pyramidal + rendering metadata).

"multiscales" is currently a MUST and there's no current proposal to change that. "omero" is non-standard and will be replaced by the rendering information itself.

@will-moore
Copy link
Contributor Author

Apologies for letting this slide...
It seems that aiming to use the OME-Zarr rendering settings to set the initial coordination state is currently a bit too ambitious for this PR. So I'm wondering if the current changes are useful enough to consider merging, or if I should close this for now and come back to it when we have a use-case or users who need it?
Either way, thanks for all your explanations and time spent on it.

@ilan-gold
Copy link
Collaborator

ilan-gold commented Jan 20, 2021

@will-moore I made a PR into your branch to set the rendering settings, except the bounds for how far the sliders can go (i.e properly having window.min and window.max). That being said I do think there is some value in this especially since we are doing work on notebooks in vitessce-python, which I think would be a nice platform for this. Maybe @keller-mark can comment? If not, I think just put an [ARCHIVED] tag in front of the title for this and we can always double back to it.

Relatedly, how are the rendering settings calculated/set for these inages? I would be interested in mocking (or outright copying) the method if possible in the browser, if you'd be ok with that. What we do now seems to work well but I'd be fine copying what you do if that is feasible.

@will-moore
Copy link
Contributor Author

When we import images into OMERO, it parses all the pixel data to find the minimum and maximum pixel values for each channel. These will be the limits of the sliders and also the initial start/end settings.

This calculation can cost a lot of time, so for very large imports (e.g. IDR) we tend to skip this at import, so the min/max values aren't in OMERO and the sliders default to the full pixel range (e.g. http://idr.openmicroscopy.org/webclient/img_detail/9621401/). In this case we tend to manually set the initial rendering setting for a large number of images (e.g. whole plate) to the same value.

When we do know min/max values our image viewers still allow the user to enter a start/end value that is outside the range of the slider (e.g. http://idr.openmicroscopy.org/webclient/img_detail/1884823/). This will "stretch" the range of the slider temporarily, until you slide a handle to a smaller range and it will return to it's original range.

OMERO also tries to guess initial channel colours from the acquisition metadata. In particular, the wavelength or filters associated with each channel. I'm not sure where this logic is, although someone else on the team will be able to show us if you're interested? (will you have access to acquisition metadata in the browser?)

@ilan-gold
Copy link
Collaborator

ilan-gold commented Jan 20, 2021

@will-moore Right now Viv exposes (and Vitessce/Avivator use) getChannelStats which uses the top level of the pyramid to calculate (approximate) channel stats at the moment such as those you mention + a reasonable guess on initial slider values. hms-dbmi/viv#358 will allow you to use any level of the pyramid though so these examples could be perfectly initialized, actually, because they are relatively small at their highest resolution. For this reason, images in Vitessce/Avivator initialize to a reasonable (but not perfect) setting without explicitly needing settings.

OMERO also tries to guess initial channel colours from the acquisition metadata. In particular, the wavelength or filters associated with each channel. I'm not sure where this logic is, although someone else on the team will be able to show us if you're interested? (will you have access to acquisition metadata in the browser?)

Is this part of the OMEXML metadata? If so, we certainly have that from ome-tiff files and could write some utilities. We have access to whatever is sent with the OME-Zarr files. hms-dbmi/viv#358 will also give us even more extensive parsing as well of the OMEXML metadata.

@jburel
Copy link

jburel commented Jan 22, 2021

@ilan-gold
Copy link
Collaborator

@will-moore I am going to close this. We have implemented support of OME-Zarr provisionally via #849 with an outstanding issue #894. We cannot set the max/min directly yet of the sliders, although the actual setting of them, colors, etc. has been dealt with. Thanks for getting the ball rolling on this, and as a bonus it prompted a good discussion of what our limitations are and made the code base stronger. I'll let you know once it is released.

@ilan-gold ilan-gold closed this Mar 19, 2021
@will-moore
Copy link
Contributor Author

Sounds great. Many thanks!

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.

None yet

6 participants