Property omexml for ZarrLoader

[andreasg123]

Could you store the omexml property in ZarrLoader like you do in OMETiffLoader (maybe as a documented property)? It contains all the metadata of the image and could be useful for an app, e.g., @_SignificantBits that would hopefully indicate 12-bit images.

[ilan-gold]

@andreasg123 Tying in @manzt but one of my Christmas break tasks is going to be hopefully unifying the two loaders under a base class of some sort. Hopefully we would tie this is, or at least the important parts of it.

[ilan-gold]

@andreasg123 To expand on this a bit, I don't think it's something that would be on the ZarrLoader as things stand since that loader is agnostic to its source (in this case OME)

[manzt]

The issue is that omexml is an OME construct and not specific to Zarr. Zarr is a generic format for N-dimensional, and the ZarrLoader is simply a wrapper over one or many Zarr.js ZarrArrays. This means that the ZarrLoader can support viewing non-OME-specific arrays, like in vizarr.

The loaders (in my opinion) should have the primary goal of "loading" array data for the layers which we provide. A viv layer never accesses loader.omexml, but derived attributes like (e.g. isRgb). I think it is a mistake to move towards inheritance here because the two cannot simply be mapped to one another. The loader should be responsible for translating metadata into the attributes needed by the layers. For example, users can write their own "loaders" for any tile source and it doesn't make sense for these to require omexml.

I'd favor some type of composition instead. For example bioformats2raw outputs data.zarr + METADATA.ome.xml. I'd prefer helper functions that return the omexml and a loader for OME-TIFF and BioformatsZarr sources:

const { loader, omexml } = createOMETiffLoader(/* ... */);
const { loader, omexml } = createBioformatsZarrLoader(/* ... */);

Here the loaders are only responsible for implementing getTile and getRaster (and a few other optional props).

[ilan-gold]

@manzt I think the advantage of inheritance would be define things we use in other places in the code base clearly so that others know to use them, like getRasterSize, isInterleaved, isRgb, getTile, numLevels etc., when writing their own loaders.

[ilan-gold]

This would allow someone to write a deepZoom loader or the like.

[ilan-gold]

I agree about the omexml though - it probably does not belong on the zarr loader.

[manzt]

@manzt I think the advantage of inheritance would be define things we use in other places in the code base clearly so that others know to use them, like getRasterSize, isInterleaved, isRgb, getTile etc., when writing their own loaders.

I'm ok with inheriting a base class that has stubs for required methods and will throw errors if not implemented. (however in this case, it would probably be more in favor of using Typescript and declaring an Loader interface ).

[ilan-gold]

@manzt I was just thinking the same about Typescript. Fully agree.

[andreasg123]

I hope that the new loader will still provide getRasterSize. That tripped me up because I expected to have width and height just like with OMETIFFLoader, causing NaN to be passed to deck.gl and leading to a very obscure stack trace.

[ilan-gold]

@andreasg123 Yes, the goal would be to prevent things like that :) My apologies for that.

[manzt]

I hope that the new loader will still provide getRasterSize. That tripped me up because I expected to have width and height just like with OMETIFFLoader, causing NaN to be passed to deck.gl and leading to a very obscure stack trace.

Yes, sorry I wasn't so clear. The I imaging interface for the Loader will contain all things that describe the array-like data that are needed for rendering. E.g., something like:

interface Loader {
  getRaster({ x, y, loaderSelection }): Promise<Data>;
  getTile({ x, y, z, loaderSelection }): Promise<Data>;
  getRasterSize({ z }): { width: number, height: number };
  isInterleaved: boolean;
  numLevels: number;
  isRgb: boolean;
}

Plus other things I'm forgetting. Essentially. We need to really iron out what a shared interface for a loader looks like, and this issue raises the point that it's currently ill-defined!

[andreasg123]

@ilan-gold, not your fault. I should have checked the documentation instead of relying on those properties to compute the zoom and center for the initial view.

If you have the time, it would be great if you could update the documentation, e.g., getRasterSize({ z }) instead of getRasterSize(z).

[andreasg123]

I just noticed that omexml appears to be responsible for the physical sizes because the same ND2 image converted to OME-TIFF and OME-Zarr only has the scale bar for the former. Hopefully, this will provide another reason to make omexml available to the viewer for Zarr images.

[ilan-gold]

@andreasg123 The OMEXML is responsible for the physical sizes but it is not used directly - the property is directly on the class, probably for this reason (being omexml agnostic): https://github.com/hms-dbmi/viv/blob/0df78f70e1271923fc1741eefa0dfbc5ff87e609/src/views/DetailView.js#L26-L29

This is another thing we'd probably want to formalize - thanks for the catch.