Ngff v0.4 #139

will-moore · 2022-01-05T16:55:31Z

Add support for v0.4 data

test with e.g. https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-05/idr0062/6001240.zarr
Deployed at:
https://deploy-preview-139--vizarr.netlify.app/?source=https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-05/idr0062/6001240.zarr

manzt · 2022-01-11T16:49:00Z

Thanks for opening this PR! Is it ready for review?

will-moore · 2022-01-12T09:16:18Z

Yes, thanks. I think the current PR is ready for review. However, the 0.4 axes spec PR is still open, so it could change (although probably not in the limited parts that affect this PR).

But I have been holding off generating larger HCS example datasets in 0.4 spec, so there aren't any public examples for testing (just been testing this PR on smaller local plates). But I could go ahead and do a medium plate if that would be useful?

Actually, I meant to ask you about supporting some of the other axes features that are added in ome/ngff#57, such as transformations (scaling and affine transforms).
I think scaling would be covered by adding a scale bar (unless the X and Y are scaled differently) but I don't know if affine transform is beyond the feature scope of vizarr?
Cheers,

manzt · 2022-01-12T15:13:06Z

Great. I'll work on the review.

Actually, I meant to ask you about supporting some of the other axes features that are added in ome/ngff#57, such as transformations (scaling and affine transforms). I think scaling would be covered by adding a scale bar (unless the X and Y are scaled differently) but I don't know if affine transform is beyond the feature scope of vizarr?

I'll need to take a look at the spec more closely, but there are two things that stand out to me which are likely related but we work on in separate PRs:

1.) Vizarr should have a scale bar
2.) Vizarr should (in theory) handle affine transformations

It's probably a bit too low-level, but affine transformations are configurable per-layer via an API we already have (https://github.com/hms-dbmi/vizarr/blob/main/example/spatial_transformations.ipynb). On a first pass, we can hopefully map the transformation defined in NGFF to this representation.

manzt

Looks great. Just some minor suggestions, thanks @will-moore !

Let's try to get this merged and then we can focus on follow-up PRs for scale bar and transformations.

manzt · 2022-01-12T15:40:30Z

src/io.ts

+  let labels = getAxisLabels(data[0], config.axis_labels);
+  let channel_axis = labels.indexOf('c');
+  if (!config.axis_labels && multiscales && multiscales[0].axes) {
+    const axes = getNgffAxes(multiscales);
+    labels = getNgffAxisLabels(axes);
+    channel_axis = axes.findIndex((axis) => axis.type === 'channel');
+  }
  const tileSize = guessTileSize(data[0]);
  const loader = data.map((d) => new ZarrPixelSource(d, labels, tileSize));
  const [base] = loader;

  // If explicit channel axis is provided, try to load as multichannel.
-  if ('channel_axis' in config || labels.includes('c')) {
+  if ('channel_axis' in config || channel_axis > -1) {
    config = config as MultichannelConfig;
-    return loadMultiChannel(config, loader, Number(config.channel_axis ?? labels.indexOf('c')));
+    return loadMultiChannel(config, loader, Number(config.channel_axis ?? channel_axis));
  }



This logic is starting to get pretty complicated. The issue is that there are multiple ways labels and channel_axis can be specified, and it's difficult to see what takes precedent over each. The way I see it:

Vizarr layer config > NGFF metadata > guessed labels

if that is the case, we could move this logic:

let data: ZarrArray[]; let labels: [...string, 'y', 'x']; let channel_axis: number = -1; if (node instance of ZarrGroup) { // ... data = await loadMultiscales(node, attrs.multiscales); if (attrs.multiscales[0].axes) { const axes = getNgffAxes(attrs.multiscales); labels = getNgffAxisLabels(axes); channel_axis = axes.findIndex((axis) => axis.type === 'channel'); } } // ... // explicit override in config > ngff > guessed from data shape labels = config.axis_labels ?? (labels ?? guessAxisLabels(data[0])); channel_axis = config.channel_axis ?? (channel_axis ?? labels.indexOf('c'));

manzt · 2022-01-12T16:00:18Z

src/utils.ts

+  if (multiscales[0].axes) {
+    axes = multiscales[0].axes.map((axis) => {
+      // axis may be string 'x' (v0.3) or object
+      let name: string = axis instanceof String ? (axis as string) : (axis as Ome.Axis).name;
+      let type = getDefaultType(name);
+      if (!(axis instanceof String) && (axis as Ome.Axis).type) {
+        type = (axis as Ome.Axis).type as string;
+      }
+      return { name, type };
+    });
+  }
+  return axes;


Lots of type assertions here which are an escape-hatch from the TS type checker (so if the types ever change this might break). Perhaps something like this would be more explicit/safe:

Suggested change

if (multiscales[0].axes) {

axes = multiscales[0].axes.map((axis) => {

// axis may be string 'x' (v0.3) or object

let name: string = axis instanceof String ? (axis as string) : (axis as Ome.Axis).name;

let type = getDefaultType(name);

if (!(axis instanceof String) && (axis as Ome.Axis).type) {

type = (axis as Ome.Axis).type as string;

}

return { name, type };

});

}

return axes;

if (multiscales[0].axes) {

axes = multiscales[0].axes.map((axis) => {

// axis may be string 'x' (v0.3) or object

if (typeof axis === 'string') {

return { name: axis, type: getDefaultType(axis) }

}

const { name, type } = axis;

return { name, type: type ?? getDefaultType(name) };

});

}

manzt · 2022-01-12T16:02:02Z

types/ome.ts

  interface Multiscale {
    datasets: { path: string }[];
    version?: string;
-    axes?: string[];
+    axes?: (string | Axis)[];


It is allowed that these types are interleaved? e.g. ['x', { name: 'y', type: 'space' }] ?

Otherwise the type here would be string[] | Axis[]

will-moore · 2022-01-13T13:26:43Z

Thanks. Those changes seem to be working well locally - think I still need to fix TypeScript...

manzt · 2022-01-13T15:43:25Z

Just checked this out and seeing how hard it is to please TS. Also the nested ternaries aren't great. What if we had a helper function for this? Keep the implementation in io.ts...

type Labels = [...string[], 'y', 'x'];
function getAxisLabelsAndChannelAxis(
  config: ImageLayerConfig,
  ngffAxes: Ome.Axis[] | undefined,
  arr: ZarrArray,
): { labels: Labels, channel_axis: number } {

	// type cast string[] to Labels
	const maybeAxisLabels = config.axis_labels as undefined | Labels;
	// ensure numeric if provided
	const maybeChannelAxis = 'channel_axis' in config ?  Number(config.channel_axis) : undefined;

	// Use ngff axes metadata if labels or channel axis aren't explicitly provided
	if (ngffAxes) {
		const labels = maybeAxisLabels ?? getNgffAxisLabels(ngffAxes);
		const channel_axis = maybeChannelAxis ?? ngffAxes.findIndex((axis) => axis.type === 'channel');
		return { labels, channel_axis }
	}

	// create dummy axis labels if not provided and try to guess channel_axis if missing
	const labels = maybeAxisLabels ?? getAxisLabels(arr);
	const channel_axis = maybeChannelAxis ?? labels.indexOf('c');
	return { labels, channel_axis }
}

and then in the main body

export async function createSourceData(config: ImageLayerConfig): Promise<SourceData> {
  const node = await open(config.source);
  let data: ZarrArray[];
  let axes: Ome.Axis[] | undefined;
  
  if (node instanceof ZarrGroup) {
    // ...
    data = await loadMultiscales(node, attrs.multiscales);
    if (attrs.multiscales[0].axes) {
      axes = getNgffAxes(attrs.multiscales);
    }
  } else {
    data = [node];
  }
  const { channel_axis, labels } = getAxisLabelsAndChannelAxis(config, axes, data[0]);
  // ...
}

Thanks manzt!

will-moore · 2022-01-14T16:29:21Z

Thanks @manzt that looks good.
It's working with all the images I've been testing on.

However, for some reason, plates have stopped working. I had them working locally earlier on this PR, but it seems that a PR change isn't responsible since I also see the same error when I revert to older commits and also at https://hms-dbmi.github.io/vizarr/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.1/plates/422.zarr
Any ideas?
Older vizarr https://hms-dbmi.github.io/vizarr/v0.1 still works with that plate.

Thanks

manzt · 2022-01-24T14:53:17Z

However, for some reason, plates have stopped working. I had them working locally earlier on this PR, but it seems that a PR change isn't responsible since I also see the same error when I revert to older commits and also at hms-dbmi.github.io/vizarr/?source=https://uk1s3.embassy.ebi.ac.uk/idr/zarr/v0.1/plates/422.zarr
Any ideas?

This is really surprising since at one point those plates where working on vizarr v0.2 & v0.3. The console errors in Chrome are very cryptic, so I tried other browsers to debug (Firefox 98 & Safari 15.1) but those actually seem to work...

What version of Chrome are you using? I'm wondering if something happened with the latest update to Chrome that for some reason breaks this layer. Have you seen this before, @ilan-gold?

will-moore · 2022-01-24T16:05:23Z

Ah, yes: https://deploy-preview-139--vizarr.netlify.app/?source=https://minio-dev.openmicroscopy.org/idr/v0.4/2022-01-24/plates/idr0004/1752.zarr is working fine for me in Firefox.
I have Chrome 97.0.4692.71 (Official Build) (x86_64)

ilan-gold · 2022-01-24T17:24:33Z

Somethign must have happened in Chrome, if you can see the error on the hms-dbmi website. That's not to say there's not something wrong with what we have, but it was working before. I do understand the error, although I can't figure out why it's being thrown since usampler2D is defined. @manzt Can you point me to a working plate for the viv 0.12 PR so I can see if that resolves things?

manzt · 2022-01-24T18:14:51Z

Somethign must have happened in Chrome, if you can see the error on the hms-dbmi website.

are all pinned builds of vizarr corresponding to the tagged releases. So to see a regression in v0.2 & v0.3 & main branch (which wasn't there before) is most likely a browser related.

@will-moore. We are working to get the latest viv release integrated in Vizarr, which has some changes to our shaders anyways. So as long as the Firefox workaround is ok at the moment, we will try to get latest Viv release added before digging into this further. It could end up that this just gets resolved with upgrading Viv/deck.gl.

will-moore added 3 commits January 5, 2022 14:28

Handle v0.4 axes names and types

309993b

Formatting fixes

4fab4a6

Use getNgffAxes() when no 'omero' metadata

954a8a0

manzt force-pushed the main branch from 0912b82 to 3f21497 Compare January 6, 2022 02:41

will-moore marked this pull request as ready for review January 6, 2022 14:42

will-moore mentioned this pull request Jan 6, 2022

Axes v0.4 ome/omero-cli-zarr#93

Merged

manzt requested changes Jan 12, 2022

View reviewed changes

Fixes suggested in PR hms-dbmi#139

cdbcc39

getAxisLabelsAndChannelAxis to fix TS

50e1a03

Thanks manzt!

manzt merged commit 1b98016 into hms-dbmi:main Jan 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ngff v0.4 #139

Ngff v0.4 #139

will-moore commented Jan 5, 2022 •

edited

manzt commented Jan 11, 2022

will-moore commented Jan 12, 2022

manzt commented Jan 12, 2022

manzt left a comment

manzt Jan 12, 2022

manzt Jan 12, 2022

manzt Jan 12, 2022

will-moore commented Jan 13, 2022

manzt commented Jan 13, 2022

will-moore commented Jan 14, 2022

manzt commented Jan 24, 2022

will-moore commented Jan 24, 2022

ilan-gold commented Jan 24, 2022

manzt commented Jan 24, 2022 •

edited

Ngff v0.4 #139

Ngff v0.4 #139

Conversation

will-moore commented Jan 5, 2022 • edited

manzt commented Jan 11, 2022

will-moore commented Jan 12, 2022

manzt commented Jan 12, 2022

manzt left a comment

Choose a reason for hiding this comment

manzt Jan 12, 2022

Choose a reason for hiding this comment

manzt Jan 12, 2022

Choose a reason for hiding this comment

manzt Jan 12, 2022

Choose a reason for hiding this comment

will-moore commented Jan 13, 2022

manzt commented Jan 13, 2022

will-moore commented Jan 14, 2022

manzt commented Jan 24, 2022

will-moore commented Jan 24, 2022

ilan-gold commented Jan 24, 2022

manzt commented Jan 24, 2022 • edited

will-moore commented Jan 5, 2022 •

edited

manzt commented Jan 24, 2022 •

edited