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

feat: Deep zoom #929

Merged
merged 3 commits into from
May 29, 2024
Merged

feat: Deep zoom #929

merged 3 commits into from
May 29, 2024

Conversation

tihuan
Copy link
Contributor

@tihuan tihuan commented May 10, 2024

This is the feature branch for the Deep Zoom feature. Deep Zoom related PRs will be merged against this branch

5/10:

Screen.Recording.2024-05-10.at.2.42.26.PM-2.mov

@@ -145,6 +146,7 @@
"@types/lodash.uniq": "^4.5.6",
"@types/lodash.zip": "^4.2.6",
"@types/node": "^20.10.6",
"@types/openseadragon": "^3.0.10",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WARNING: There's only v3 types, but we're using v4 library 😭

There's a ticket for this: https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-explorer/899

@@ -34,7 +34,7 @@ import { packDiffExPdu, DiffExMode, DiffExArguments } from "../util/diffexpdu";
import { track } from "../analytics";
import { EVENTS } from "../analytics/events";
import AnnoMatrix from "../annoMatrix/annoMatrix";
import { checkFeatureFlags } from "../util/featureFlags/featureFlags";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move checkFeatureFlags to /src/index.tsx, so we do the check ASAP

// save isCellGuideCxg to the reducer store
dispatch({ type: "initial data load complete", isCellGuideCxg });
// save `isCellGuideCxg` and `s3URI` to the reducer store
dispatch({ type: "initial data load complete", isCellGuideCxg, s3URI });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding s3URI to the store, since now we need to parse s3URI to find the right deep zoom assets in S3

import { spatialEmbeddingKeyword } from "../globals";
import { RootState } from "../reducers";

export function isSpatialMode(props: Partial<RootState>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a new selectors file to add selectors

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we really should type our state at some point. RootState is just a masked any

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is safe to remove now. But there's probably more MVP related code that needs to be removed in a subsequent PR

Ticket here: https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-explorer/894

this.canvas,
clientX,
clientY,
projectionInvTF
);
this.zoomAt(1 / Math.exp(scale / height), pos[0], pos[1]);

this.zoomAt({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing more args to zoom, since OSD needs the info

viewChanged = this.wheelZoom(
e as unknown as WheelEvent,
projectionTF,
openseadragon
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

passing openseadragon instance here

Comment on lines +31 to +39
const params = getParams();

const paramValue = params?.get(key);

if (paramValue !== undefined) {
setFeatureFlag(key, paramValue || "");
return paramValue === ParamBoolean.TRUE;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolves the following bug:

  1. Clear local storage
  2. Load http://localhost:3000/d/super-cool-spatial.cxg?spatial=true&dl=true
  3. getFeatureFlag("spatial") actually returns false instead of true, because the getter is called before the setter

The solution is to prioritize param as the source of truth for feature flag state, and also set the local storage proactively

Comment on lines 56 to 71
# Preparing data for pyvips from the cropped numpy array

# NOTE: Flip the image vertically, since somehow the client flips the image
image_pil = image_pil.transpose(Image.FLIP_TOP_BOTTOM)

# Convert the flipped PIL Image back to a numpy array
flipped_array_uint8 = np.array(image_pil)

h, w, bands = flipped_array_uint8.shape
linear = flipped_array_uint8.reshape(w * h * bands)
vipsImage = pyvips.Image.new_from_memory(linear.data, w, h, bands, "uchar")

# Save as Deep Zoom Image (this will create a directory with tiles and a .dzi file)
vipsImage.dzsave("./server/common/web/static/deep_zoom/spatial", suffix=".jpeg")

h = w = min(h, w) # Adjust for 1:1 aspect ratio
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code to generate deep zoom assets locally. We have another ticket to clean this up here:

https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/gh/chanzuckerberg/single-cell-explorer/920

server/common/utils/uns.py Outdated Show resolved Hide resolved
@tihuan tihuan requested review from codemonkey800, seve and kaloster May 13, 2024 14:21
@tihuan tihuan changed the title feat: Deep zoom [DO NOT MERGE] feat: Deep zoom May 14, 2024
import { spatialEmbeddingKeyword } from "../globals";
import { RootState } from "../reducers";

export function isSpatialMode(props: Partial<RootState>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we really should type our state at some point. RootState is just a masked any

Comment on lines +113 to +112
const fractionToUse = isSpatialMode ? 1 : 0.95; // fraction of min dimension to use
const topGutterSizePx = isSpatialMode ? 0 : 32; // top gutter for tools
const bottomGutterSizePx = isSpatialMode ? 0 : 32; // bottom gutter for tools
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to conduct extra QA of centroid labels

return `${url}spatial.dzi`;
}

function getSpatialUrl(s3URI: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just provide this URL on /config like we do with similar pieces of information

@tihuan tihuan changed the title [DO NOT MERGE] feat: Deep zoom feat: Deep zoom May 29, 2024
@tihuan
Copy link
Contributor Author

tihuan commented May 29, 2024

discussed with the team that we have enough e2e test coverage to merge this feature branch into main now to get more days for stakeholders to test the feature. And I will write more e2e tests this week to cover new features added! #yolo

@tihuan tihuan merged commit 164aa76 into main May 29, 2024
4 of 5 checks passed
@tihuan tihuan deleted the feat-deep-zoom branch May 29, 2024 17:21
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.

4 participants