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 refactor #852

Merged
merged 34 commits into from
Apr 3, 2024
Merged

feat: spatial refactor #852

merged 34 commits into from
Apr 3, 2024

Conversation

kaloster
Copy link
Contributor

@kaloster kaloster commented Mar 26, 2024

This addresses the following:

  • Refactor of code
  • Consolidation of endpoints to uns/meta
  • Re-converted super-cool-spatial
  • Added unit test for uns endpoint

Tested for following scenarios -

  • Dataset with spatial embedding and uns array
  • Dataset with spatial embedding without uns array (pre-migration)
  • Dataset without spatial embedding

All above tested with feature flag on and off along with image download

Screen.Recording.2024-04-01.at.7.35.28.PM.mov

Copy link

codecov bot commented Apr 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.58%. Comparing base (e86f4a6) to head (9752bbe).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #852   +/-   ##
=======================================
  Coverage   76.57%   76.58%           
=======================================
  Files          88       88           
  Lines        6994     7020   +26     
=======================================
+ Hits         5356     5376   +20     
- Misses       1638     1644    +6     
Flag Coverage Δ
frontend 76.58% <ø> (+<0.01%) ⬆️
javascript 76.58% <ø> (+<0.01%) ⬆️
unitTest 76.58% <ø> (+<0.01%) ⬆️

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.

@kaloster kaloster requested review from seve and tihuan April 1, 2024 17:15
@kaloster kaloster marked this pull request as ready for review April 1, 2024 17:20
@tihuan
Copy link
Contributor

tihuan commented Apr 1, 2024

@kaloster seems like subsetting feature is actually broken, so e2e tests are correctly failing! I'm hoping once the bug is fixed all e2e tests will pass 🤞

Thank you!

CC: @seve

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.

LGTM! Just some minor comments I can contribute lol Thanks so much for carrying the refactor through the finish line, Ronen! 👏 🙏 🚀 !!

client/__tests__/util/stateManager/sampleResponses.ts Outdated Show resolved Hide resolved
@@ -15,6 +15,7 @@
"e2e-dev": "CXG_URL_BASE=https://cellxgene.dev.single-cell.czi.technology playwright test",
"e2e-stage": "CXG_URL_BASE=https://cellxgene.staging.single-cell.czi.technology playwright test",
"e2e-prod": "CXG_URL_BASE=https://cellxgene.cziscience.com playwright test",
"e2e-copy-snapshots-to-linux": "for file in __tests__/e2e/e2e.test.ts-snapshots/*-darwin.txt; do cp \"$file\" \"${file/-darwin.txt/-linux.txt}\"; done",
Copy link
Contributor

Choose a reason for hiding this comment

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

PSA: Use npm run e2e-copy-snapshots-to-linux to copy updated darwin snapshots to linux equivalents!

server/tests/unit/dataset/test_cxg_dataset.py Outdated Show resolved Hide resolved
client/src/components/graph/graph.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@seve seve left a comment

Choose a reason for hiding this comment

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

Non-blocking nits, should we create a ticket to investigate moving the dataset metadata ("corpora_props") to the uns loader rather than the config endpoint?

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

tihuan commented Apr 2, 2024

e2e all passing now 🤩 👏 !! Thanks for the amazing work, everyone!!

@kaloster kaloster requested review from tihuan and seve April 3, 2024 15:55
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.

Looks really good! Thanks so much, Ronen 🙏 Just two small comments otherwise LGTM!

client/src/reducers/controls.ts Outdated Show resolved Hide resolved
server/tests/unit/common/apis/test_api_v3.py Show resolved Hide resolved
@tihuan
Copy link
Contributor

tihuan commented Apr 3, 2024

Don't worry about the UI Tests part! I rejected some snapshots because the current ones are better than the PR ones

client/src/common/types/entities.ts Outdated Show resolved Hide resolved
client/src/components/graph/graph.tsx Outdated Show resolved Hide resolved
client/src/components/graph/graph.tsx Outdated Show resolved Hide resolved
@kaloster kaloster merged commit ffd7281 into main Apr 3, 2024
6 of 7 checks passed
@kaloster kaloster deleted the kaloster/spatial-refactor branch April 3, 2024 19:36
@kaloster
Copy link
Contributor Author

kaloster commented Apr 4, 2024

tagging @atarashansky if you could look and comment on the b/e implementation 🙏🏽

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