warn annotation loss with resolution change #977
Conversation
Signed-off-by: Alex Pickering <[email protected]>
Signed-off-by: Alex Pickering <[email protected]>
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖 One Page Changed SizeThe following page changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. |
Signed-off-by: Alex Pickering <[email protected]>
Signed-off-by: Alex Pickering <[email protected]>
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
569.13 KB (🟡 +64 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
One Page Changed Size
The following page changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/experiments/[experimentId]/data-processing |
537.24 KB (-40 B) |
1.08 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Signed-off-by: Alex Pickering <[email protected]>
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
569.13 KB (🟡 +64 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
One Page Changed Size
The following page changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/experiments/[experimentId]/data-processing |
537.24 KB (-40 B) |
1.08 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #977 +/- ##
==========================================
+ Coverage 85.09% 85.20% +0.10%
==========================================
Files 548 548
Lines 10092 10083 -9
Branches 2131 2174 +43
==========================================
+ Hits 8588 8591 +3
+ Misses 1445 1432 -13
- Partials 59 60 +1 ☔ View full report in Codecov by Sentry. |
src/redux/actions/pipeline/runQC.js
Outdated
| const embeddingChanged = changedQCFilters.has('embeddingSettings'); | ||
| const clusteringChanged = changedQCFilters.has('clusteringSettings'); | ||
| const otherChanged = [...changedQCFilters].some((value) => value !== 'embeddingSettings' && value !== 'clusteringSettings'); | ||
|
|
||
| // if only embedding or clustering changed | ||
| if (!otherChanged) { | ||
| await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding')); |
There was a problem hiding this comment.
| const embeddingChanged = changedQCFilters.has('embeddingSettings'); | |
| const clusteringChanged = changedQCFilters.has('clusteringSettings'); | |
| const otherChanged = [...changedQCFilters].some((value) => value !== 'embeddingSettings' && value !== 'clusteringSettings'); | |
| // if only embedding or clustering changed | |
| if (!otherChanged) { | |
| await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding')); | |
| const embeddingChanged = changedQCFilters.has('embeddingSettings'); | |
| const clusteringChanged = changedQCFilters.has('clusteringSettings'); | |
| if (embeddingChanged || clusteringChanged) { | |
| await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding')); | |
There was a problem hiding this comment.
Also if you move back the await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding')); line to the runOnlyClustering and runOnlyConfigureEmbedding functions, this whole block can be only:
const embeddingChanged = changedQCFilters.has('embeddingSettings');
const clusteringChanged = changedQCFilters.has('clusteringSettings');
if (embeddingChanged) {
runOnlyConfigureEmbedding(
experimentId,
processing.configureEmbedding.embeddingSettings.method,
dispatch,
);
}
if (clusteringChanged) {
runOnlyClustering(
experimentId,
processing.configureEmbedding.clusteringSettings.methodSettings.louvain.resolution,
dispatch,
);
}
There was a problem hiding this comment.
So now that means that if there's more than one changed filter, running it with changed embedding or clustering in the last step will discard them all and run only embedding / clustering?
The problem with this is that it will run the conditional block if there are other changes to Data Processing in addition to an embedding or clustering change. I want the condition to run if there is only an embedding or clustering change (or both but nothing else). Embedding and clustering will be run after the pipeline finishes if there are any other changes so I don't want to call embedding/clustering in those cases.
There was a problem hiding this comment.
Also if you move back the
await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding'));line to therunOnlyClusteringandrunOnlyConfigureEmbeddingfunctions, this whole block can be only:const embeddingChanged = changedQCFilters.has('embeddingSettings'); const clusteringChanged = changedQCFilters.has('clusteringSettings'); if (embeddingChanged) { runOnlyConfigureEmbedding( experimentId, processing.configureEmbedding.embeddingSettings.method, dispatch, ); } if (clusteringChanged) { runOnlyClustering( experimentId, processing.configureEmbedding.clusteringSettings.methodSettings.louvain.resolution, dispatch, ); }
happy to move the line back if you have a preference. I moved it out so that it wasn't duplicated between the two functions
There was a problem hiding this comment.
I was only thinking in case we want to use the runOnlyClustering and runOnlyConfigureEmbedding functions somewhere else in the code, someone might expect them to do a save and forget to dispatch(save before running them.
| } | ||
| // updates to configure embedding run on worker if they are the only changes | ||
| // need to know if change was to embedding or clustering settings | ||
| const settingType = Object.keys(diff)[0]; |
There was a problem hiding this comment.
Why only the first changed from the diff and not the whole Object.keys(diff)?
There was a problem hiding this comment.
Each of the diffs is a single object where the top level key is either going to be embeddingSettings or clusteringSettings. This just extracts that key.
| @@ -42,12 +56,27 @@ const runQC = (experimentId) => async (dispatch, getState) => { | |||
| const { processing } = getState().experimentSettings; | |||
| const { changedQCFilters } = processing.meta; | |||
|
|
|||
| if (changedQCFilters.size === 1 && changedQCFilters.has('configureEmbedding')) { | |||
There was a problem hiding this comment.
With changedQCFilters.size === 1 removed, now that means that if there's more than one changed filter, running it with changed embedding or clustering in the last step will discard them all and run only embedding / clustering?
There was a problem hiding this comment.
changedQCFilters.size === 1 is removed because we will now be looking for either exactly embeddingSettings , clusteringSettings, or both. If both are changed but nothing else we also need to run this conditional block (so size is no longer a good way to distinguish).
There was a problem hiding this comment.
Sorry I cant test in staging because i dont have an hms account but what i see is that now this will be run even if there are other changedQCFilters from previous steps. There is a return on line 81, so that means qc wont be triggered.
What will happen if i change for example the mitochondrial content filter and then go to configure embedding and change clustering / embedding there? I think mitochondrial filter changes will be discarded and clustering / embedding will be ran right?
There was a problem hiding this comment.
If you send me an email I'll create an account for you to test. Here is why it doesn't run if there are other steps changed:
const otherChanged = [...changedQCFilters].some((value) => value !== 'embeddingSettings' && value !== 'clusteringSettings');This checks to see if there are other settings changed. If there are not, then the below conditional block runs. See added comments to hopefully clarify:
if (!otherChanged) {
// this runs because exactly embedding or clustering or both has changed (but nothing else)
await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding'));
if (embeddingChanged) {
runOnlyConfigureEmbedding(
experimentId,
processing.configureEmbedding.embeddingSettings.method,
dispatch,
);
}
if (clusteringChanged) {
runOnlyClustering(
experimentId,
processing.configureEmbedding.clusteringSettings.methodSettings.louvain.resolution,
dispatch,
);
}
// we return because we are in the !otherChanged block so nothing else changed and we don't re-run QC
return;
}There was a problem hiding this comment.
Not at all -- thank you -- always good to have a careful review :)
Signed-off-by: Alex Pickering <[email protected]>
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
569.14 KB (🟡 +70 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
One Page Changed Size
The following page changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/experiments/[experimentId]/data-processing |
537.24 KB (-27 B) |
1.08 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
📦 Next.js Bundle Analysis for uiThis analysis was generated by the Next.js Bundle Analysis action. 🤖
|
| Page | Size (compressed) |
|---|---|
global |
569.14 KB (🟡 +69 B) |
Details
The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!
One Page Changed Size
The following page changed size from the code in this PR compared to its base branch:
| Page | Size (compressed) | First Load |
|---|---|---|
/experiments/[experimentId]/data-processing |
537.25 KB (-14 B) |
1.08 MB |
Details
Only the gzipped size is provided here based on an expert tip.
First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.
Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis
Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.
Description
Makes it so that resolution changes also require confirmation before they are run. Also adds a warning that re-running QC will cause loss of annotations.
staging:
https://ui-alex-ui977.staging.single-cell-platform.net/
Details
URL to issue
hms-dbmi-cellenics/issues#7
Link to staging deployment URL (or set N/A)
N/A
Links to any PRs or resources related to this PR
Integration test branch
master
Merge checklist
Your changes will be ready for merging after all of the steps below have been completed.
Code updates
Have best practices and ongoing refactors being observed in this PR
Manual/unit testing
Integration testing
You must check the box below to run integration tests on the latest commit on your PR branch.
Integration tests have to pass before the PR can be merged. Without checking the box, your PR
will not pass the required status checks for merging.
Documentation updates
Optional