Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new dimensionality reduction computation plugin and enhances the scatterplot visualization to support computed variables in backend requests. The implementation follows the pattern of similar plugins like beta diversity.
- Adds a
dimensionalityReductioncomputation plugin with PCA axis support - Introduces
sendComputedVariablesInRequestoption to control whether computed variable descriptors are sent to the backend - Registers the new plugin in the computation plugins index
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/libs/eda/src/lib/core/components/visualizations/implementations/ScatterplotVisualization.tsx | Adds sendComputedVariablesInRequest option and uses it to conditionally send computed X/Y axis descriptors to backend |
| packages/libs/eda/src/lib/core/components/computations/plugins/index.ts | Registers the new dimensionalityreduction plugin |
| packages/libs/eda/src/lib/core/components/computations/plugins/dimensionalityReduction.tsx | New computation plugin for dimensionality reduction (PCA) with configuration UI and computed axis details |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...libs/eda/src/lib/core/components/visualizations/implementations/ScatterplotVisualization.tsx
Outdated
Show resolved
Hide resolved
packages/libs/eda/src/lib/core/components/computations/plugins/dimensionalityReduction.tsx
Outdated
Show resolved
Hide resolved
packages/libs/eda/src/lib/core/components/computations/plugins/dimensionalityReduction.tsx
Outdated
Show resolved
Hide resolved
packages/libs/eda/src/lib/core/components/computations/plugins/dimensionalityReduction.tsx
Outdated
Show resolved
Hide resolved
packages/libs/eda/src/lib/core/components/computations/plugins/dimensionalityReduction.tsx
Outdated
Show resolved
Hide resolved
packages/libs/eda/src/lib/core/components/computations/plugins/dimensionalityReduction.tsx
Outdated
Show resolved
Hide resolved
|
More testing notes - the PR as it is can only be tested 1. with mbio and 2. in the regular eda workspace (not the notebook). |
|
Tagging @bobular |
bobular
left a comment
There was a problem hiding this comment.
This looks good. A few minor comments we can discuss if needed. I assume it's all working, but I haven't tested.
Is there another PR where this is integrated into a notebook?
| return { | ||
| entityId: config.collectionVariable.entityId, | ||
| placeholderDisplayName: 'PCA Axis 1', | ||
| variableId: 'PC1', |
There was a problem hiding this comment.
So we just "know" that "PC1" is a valid variableId here? This is a computed (not curated) variable collection, so I guess "PC%d" is defined on the back end somewhere. It's not going to change, I guess. Just wanted to understand better. I'm not demanding hugely complex changes to make this more bulletproof than it needs to be!
There was a problem hiding this comment.
Yes, for right now we just "know", and yep you're right it's defined in the backend. The differential expression function in veupathutils returns multiple PCs and you can even tell it how many to return. I had started to implement support for choosing PCs but then left it alone because it felt like it was making the initial deliverable too big.
| xAxisVariable: | ||
| sendComputedVariablesInRequest && computedXAxisDescriptor | ||
| ? computedXAxisDescriptor | ||
| : vizConfig.xAxisVariable, |
There was a problem hiding this comment.
Line 382 (variablesForConstraints) should probably be calculated consistently too? (including sendComputedVariablesInRequest in the conditional?
e.g.
xAxisVariable: sendComputedVariablesInRequest && computedXAxisDescriptor
? computedXAxisDescriptor
: vizConfig.xAxisVariable,
* add draft dimensionality reduction compute * working pca plot * add sendComputedVariablesInRequest to Options * cleanup * cleanup * dual compute notebook success * show step number and helper text for pca * update differential expression notebook to include pca * ensure consistent calculation of axis variables * temporarily turn off differentialexpression
For the differential expression notebook.
This PR works with PR 75 in
service-eda. Can use qa mbio to test and see example requests in that PR.Tasks
Notes:
Can't use the scatterplot straight up because the dim red plot needs to set its own x/y axes with the computed variable (in beta div, the client doesn't set x/y. it just gets only that stuff from the backend). So I used the
Optionsfeature to have the new computedimensionalityReductiontellScatterplotVisualizationthat it wants to use these computed variables and that these computed vars need to be used in the request to the backend. We could have done it more simply with some hard coding but this will make it easier for us to add the ability to choose which principle coordinate the user wants for each axis. The backend is already prepped for this new feature, so the frontend might as well be, too.Testing (updated 11/04/25)
yarn ... /eda).IMPORTANT - do not merge with index.ts still including this compute. The backend has mbio approved for this compute because we need it for data testing. We can merge all this work but we must take it out of index.ts first so that it doesn't accidentally show up on the mbio live site someday on accident 😬