Feature/zarr thumbnails #143

BrianWhitneyAI · 2024-06-25T19:12:04Z

Description

The purpose of this PR is to render thumbnails for zarr images. Resolves #89. This includes finding the lowest resolution version and taking a middle slice to ensure the highest possibility of a useful thumbnail.

Issues

This feature is heavily dependent on the access level of the file, often failing with CORS issues. I believe this is all server-side, so if we want to have this feature usable with FMS data we need to adjust those permissions.

Testing

Since we cannot test on existing data in FMS we need to import our data. I used the variance dataset to check this. I was only able to import this dataset successfully on Mac so there still might be some weird things here. If you use the variance dataset then make sure to remove the thumbnail column.

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts

aswallace · 2024-06-26T17:16:41Z

packages/core/entity/FileDetail/index.ts

+                    return await renderZarrThumbnailURL(this.path);
+                } catch (error) {
+                    console.error("Error generating Zarr thumbnail:", error);
+                    throw new Error("Unable to generate Zarr thumbnail");


I'm curious if we need both the console error and the throw here

removed in 552170f 552170f 552170f

I think these are a symptom of lots of iterations trying to read Zarr images and adding tries to bypass some of the errors

Definitely think error handling is good move, just that having both throw & log seemed redundant

What would we want to add in error handling here? Additional context that it comes from trying to render the image? The error is re-raised previously with the added console log so my thought was just let the error show if something was broken.

aswallace · 2024-06-26T17:18:02Z

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts

+        return canvas.toDataURL("image/png");
+    } catch (error) {
+        console.error("Error reading Zarr image:", error);
+        throw error;


Same question here, both console and throw needed?

removed in 552170f 552170f 552170f

aswallace · 2024-06-26T17:25:01Z

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts

tiny nit -- feels like this .ts file name should start with lowercase?

Is this consistent in our repo? I tried to match to other files though admittedly those are .tsx files so I may be missing some rule.

In general I've been seeing .tsx and class files capitalized, .ts/util files lower case, but I'm not sure how consistent that is

SeanLeRoy · 2024-06-26T17:34:41Z

Were you able to test out NucMorph files? I sent you one previously via Slack - it would be cool to know it works on other ZARRs beyond those in variance

SeanLeRoy · 2024-06-26T17:35:21Z

packages/core/components/FileDetails/index.tsx

@@ -78,14 +78,20 @@ function resizeHandleDoubleClick() {
 export default function FileDetails(props: Props) {
    const [windowState, windowDispatch] = React.useReducer(windowStateReducer, INITIAL_STATE);
    const [fileDetails, isLoading] = useFileDetails();
+    const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>(undefined);


undefined is implicit

Suggested change

const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>(undefined);

const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>();

resolved in 552170f 552170f 552170f

SeanLeRoy · 2024-06-26T17:35:27Z

packages/core/components/FileDetails/index.tsx

@@ -78,14 +78,20 @@ function resizeHandleDoubleClick() {
 export default function FileDetails(props: Props) {
    const [windowState, windowDispatch] = React.useReducer(windowStateReducer, INITIAL_STATE);
    const [fileDetails, isLoading] = useFileDetails();
+    const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>(undefined);


undefined is implicit

Suggested change

const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>(undefined);

const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>();

resolved in 552170f 552170f 552170f

SeanLeRoy · 2024-06-26T17:36:45Z

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts

@@ -0,0 +1,101 @@
+import * as zarr from "@zarrita/core";
+import { FetchStore } from "@zarrita/storage";


I think all of these are used. Am I missing something? zarr is used for accessing the root. FetchStore is used for accessing the store from url and zarrGet is used to create a lowest resolution view.

SeanLeRoy · 2024-06-26T17:40:46Z

packages/core/entity/FileDetail/index.ts

+            }
+            if (this.cloudPath.endsWith(".zarr")) {
+                try {
+                    if (this.path?.startsWith("/allen")) {


I don't think the code could ever make it here for this condition because of the return on line 168 checking for the same condition.

resolved in 552170f 552170f 552170f

SeanLeRoy · 2024-06-26T17:42:42Z

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts

+    }));
+}
+
+export async function renderZarrThumbnailURL(zarrUrl: string): Promise<string> {


Could you add a comment describing this function and/or more comments across the function to explain the various things going on?

added in bde338a bde338a bde338a

BrianWhitneyAI · 2024-06-26T21:46:14Z

Were you able to test out NucMorph files? I sent you one previously via Slack - it would be cool to know it works on other ZARRs beyond those in variance

I tried it with this one https://allencell.s3.amazonaws.com/aics/nuc_morph_data/data_for_analysis/baseline_colonies/20200323_09_small/seg.ome.zarr and the image rendered to

SeanLeRoy

LGTM I'm excited to try this out with everything else altogether

aswallace

Works for me locally! Below are thoughts that don't need to be addressed rn:

My only concern is that this requires a lot of network calls at once, including multiple expected non-breaking 404s per file. The main build up is on initial load, which makes sense, but also happens if you just click away from a file that's already loaded once and click back into it, so they can multiply quickly and cause some slowness. I'm wondering if there is some way we can cache...something?, but not sure if that's a realistic possibility.

Here's the network log after clicking into one file after clearing logs from initial page load:

aswallace · 2024-07-03T23:36:11Z

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts

In general I've been seeing .tsx and class files capitalized, .ts/util files lower case, but I'm not sure how consistent that is

aswallace · 2024-07-03T23:49:05Z

packages/core/components/FileList/LazilyRenderedThumbnail.tsx

+    const [thumbnailPath, setThumbnailPath] = React.useState<string | undefined>();
+    React.useEffect(() => {
+        if (file) {
+            file.getPathToThumbnail().then(setThumbnailPath);


Since this can take some time w/ the zarr network calls, we might consider adding a loading icon specific to the thumbnail, especially since otherwise it can display the wrong thumbnail for a while

aswallace · 2024-07-03T23:54:19Z

packages/core/entity/FileDetail/index.ts

@@ -160,24 +161,26 @@ export default class FileDetail {
        return this.fileDetail.annotations.find((annotation) => annotation.name === annotationName);
    }

-    public getPathToThumbnail(): string | undefined {
+    public async getPathToThumbnail(): Promise<string | undefined> {
+        // If the thumbnail is a relative path on the allen drive then preprend it to


nit, old unrelated typo: prepend

BrianWhitneyAI added 2 commits June 18, 2024 10:37

initial commit

2a96f23

update pathing

6b7da56

BrianWhitneyAI requested review from SeanLeRoy and aswallace June 25, 2024 19:49

BrianWhitneyAI self-assigned this Jun 26, 2024

aswallace reviewed Jun 26, 2024

View reviewed changes

packages/core/entity/FileDetail/RenderZarrThumbnailURL.ts Outdated Show resolved Hide resolved

aswallace reviewed Jun 26, 2024

View reviewed changes

SeanLeRoy reviewed Jun 26, 2024

View reviewed changes

BrianWhitneyAI added 2 commits June 26, 2024 14:04

comment resolution

552170f

comments

bde338a

BrianWhitneyAI requested review from SeanLeRoy and aswallace June 26, 2024 21:48

SeanLeRoy approved these changes Jul 1, 2024

View reviewed changes

aswallace approved these changes Jul 4, 2024

View reviewed changes

This was referenced Jul 5, 2024

Loading Icon for rendered Zarr images #161

Closed

Reduce network calls for zarr rendering #162

Closed

BrianWhitneyAI and others added 2 commits July 5, 2024 11:28

Merge branch 'main' into feature/zarr-thumbnails

a4754b5

adjust pathing after merge conflict

5d8aad1

BrianWhitneyAI merged commit cdff2cb into main Jul 5, 2024
3 checks passed

BrianWhitneyAI deleted the feature/zarr-thumbnails branch July 5, 2024 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/zarr thumbnails #143

Feature/zarr thumbnails #143

BrianWhitneyAI commented Jun 25, 2024 •

edited

Loading

aswallace Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

aswallace Jun 26, 2024

BrianWhitneyAI Jun 27, 2024

aswallace Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

aswallace Jun 26, 2024

BrianWhitneyAI Jul 2, 2024

aswallace Jul 3, 2024

SeanLeRoy commented Jun 26, 2024

SeanLeRoy Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

SeanLeRoy Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

SeanLeRoy Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

SeanLeRoy Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

SeanLeRoy Jun 26, 2024

BrianWhitneyAI Jun 26, 2024

BrianWhitneyAI commented Jun 26, 2024

SeanLeRoy left a comment

aswallace left a comment

aswallace Jul 3, 2024

aswallace Jul 3, 2024

aswallace Jul 3, 2024

	const [thumbnailPath, setThumbnailPath] = React.useState<string \| undefined>(undefined);
	const [thumbnailPath, setThumbnailPath] = React.useState<string \| undefined>();

		@@ -0,0 +1,101 @@
		import * as zarr from "@zarrita/core";
		import { FetchStore } from "@zarrita/storage";

Feature/zarr thumbnails #143

Feature/zarr thumbnails #143

Conversation

BrianWhitneyAI commented Jun 25, 2024 • edited Loading

Description

Issues

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanLeRoy commented Jun 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianWhitneyAI commented Jun 26, 2024

SeanLeRoy left a comment

Choose a reason for hiding this comment

aswallace left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrianWhitneyAI commented Jun 25, 2024 •

edited

Loading