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: Spatial Mode User Story 1 #805

Merged
merged 22 commits into from
Mar 8, 2024
Merged

Conversation

kaloster
Copy link
Contributor

@kaloster kaloster commented Mar 4, 2024

  • Added all features based on cellxgene POC and adjusted to explorer
  • Added feature flag
  • Added default imageUnderlay switch when viewing spatial embedding
  • Changed to webp format for image underlay

To test:

  • Pull branch and add the test database (either locally or via the direct S3 URI (to be added by Seve)
  • Run explorer
  • Visit the frontend and add the feature flag query param -
    http://localhost:3000/d/ba344978-e1aa-40db-a611-b952c10df148.cxg?spatial=true

Still to do in separate PRs:

  • Add different spatial embedding resolutions to embedding drawer
  • Add proper typing where any
  • Unit and e2e tests
Screen.Recording.2024-03-04.at.3.48.37.PM.mov

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

Attention: Patch coverage is 40.47619% with 50 lines in your changes are missing coverage. Please review.

Project coverage is 76.57%. Comparing base (d2e1ea2) to head (1bc470c).

Files Patch % Lines
server/common/rest.py 21.21% 26 Missing ⚠️
server/dataset/dataset.py 50.00% 17 Missing ⚠️
server/dataset/cxg_dataset.py 16.66% 5 Missing ⚠️
server/app/api/v3.py 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #805      +/-   ##
==========================================
- Coverage   77.47%   76.57%   -0.90%     
==========================================
  Files          88       88              
  Lines        6856     6994     +138     
==========================================
+ Hits         5312     5356      +44     
- Misses       1544     1638      +94     
Flag Coverage Δ
frontend 76.57% <40.47%> (-0.90%) ⬇️
javascript 76.57% <40.47%> (-0.90%) ⬇️
unitTest 76.57% <40.47%> (-0.90%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

client/src/actions/index.ts Outdated Show resolved Hide resolved
@kaloster kaloster marked this pull request as ready for review March 4, 2024 19:21
@kaloster kaloster requested review from tihuan and seve March 4, 2024 21:02

if (!prevConditionMet && currentConditionMet) {
dispatch({
type: "toggle image underlay",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Toggle image underlay on update if Spatial embedding exists and currently selected

client/src/actions/index.ts Outdated Show resolved Hide resolved
client/src/actions/index.ts Show resolved Hide resolved
client/src/components/graph/drawSpatialImageRegl.ts Outdated Show resolved Hide resolved
client/__tests__/e2e/e2e.test.ts Outdated Show resolved Hide resolved
@@ -15,6 +15,7 @@ gunicorn[gevent]==20.0.4
numba==0.56.4
numpy==1.23.5
pandas==1.5.3
pillow==9.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this new package for? Thanks!

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 is PIL/Pillow used to generate the spatial image in the backend based on the RGB array and return in any image format, in our case webp

Copy link
Contributor

@tihuan tihuan left a comment

Choose a reason for hiding this comment

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

Just reviewed the PR 🙏 ✨

All looks super good and I could only offer tiny nits 😆

Thanks again for the wonderful work, Ronen!

Since we're aiming to merge the PR on Friday, no rush to address it today. I know it's late in EST 👍

Have a great evening!

client/src/components/embedding/index.tsx Outdated Show resolved Hide resolved
client/src/components/embedding/index.tsx Outdated Show resolved Hide resolved
client/src/components/graph/drawSpatialImageRegl.ts Outdated Show resolved Hide resolved
client/src/components/graph/graph.tsx Outdated Show resolved Hide resolved
client/src/components/graph/graph.tsx Show resolved Hide resolved
client/src/components/graph/graph.tsx Show resolved Hide resolved
client/src/components/menubar/index.tsx Outdated Show resolved Hide resolved
client/src/components/menubar/index.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,24 @@
import { Action } from "redux";
Copy link
Contributor

Choose a reason for hiding this comment

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

this can probably just be moved to controls

client/__tests__/e2e/e2e.test.ts Outdated Show resolved Hide resolved
client/src/components/graph/graph.tsx Show resolved Hide resolved
@tihuan
Copy link
Contributor

tihuan commented Mar 7, 2024

@seve just wanted to double check that this is no longer an issue:

#805 (comment)

Thank you!

@kaloster kaloster requested review from seve and tihuan March 8, 2024 17:01
client/__tests__/e2e/e2e.test.ts Outdated Show resolved Hide resolved
@kaloster kaloster merged commit df7d42d into main Mar 8, 2024
6 of 7 checks passed
@kaloster kaloster deleted the kaloster/spatial-user-story-1 branch March 8, 2024 21:50
/*
Map an XY coordinate from cell/point domain to screen range. Inverse
of mapScreenToPoint()
*/
const { camera, projectionTF, modelTF, viewport } = this.state;
const cameraTF = camera!.view();
const cameraTF = camera?.view() || undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

since we have optional chaining already, we don't have to do || defined, since TS transpiles camera?.view() to camera ? camera.view() : camera in JS automatically

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.

3 participants