Merge pull request #977 from hms-dbmi-cellenics/warn-annotation-loss

warn annotation loss with resolution change
hms-dbmi-cellenics · Feb 13, 2024 · 7e674c3 · 7e674c3
2 parents a4c16b5 + 69f0bcb
commit 7e674c3
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 66 deletions.
diff --git a/src/__test__/redux/actions/pipelines/__snapshots__/runQC.test.js.snap b/src/__test__/redux/actions/pipelines/__snapshots__/runQC.test.js.snap
@@ -24,7 +24,19 @@ exports[`runQC action Dispatches events properly 2`] = `
 
 exports[`runQC action Dispatches status error if loading fails 1`] = `[]`;
 
-exports[`runQC action Runs only the embedding if only changed filter was configureEmbedding 1`] = `
+exports[`runQC action Runs only clustering if only changed filter was clusteringSettings 1`] = `
+[
+  {
+    "payload": {},
+    "type": "experimentSettings/discardChangedQCFilters",
+  },
+  {
+    "type": "cellSets/clusteringUpdating",
+  },
+]
+`;
+
+exports[`runQC action Runs only the embedding if only changed filter was embeddingSettings 1`] = `
 [
   {
     "payload": {},

diff --git a/src/__test__/redux/actions/pipelines/runQC.test.js b/src/__test__/redux/actions/pipelines/runQC.test.js
@@ -13,6 +13,8 @@ import {
   EXPERIMENT_SETTINGS_DISCARD_CHANGED_QC_FILTERS,
 } from 'redux/actionTypes/experimentSettings';
 
+import { CELL_SETS_CLUSTERING_UPDATING } from 'redux/actionTypes/cellSets';
+
 import { EMBEDDINGS_LOADING } from 'redux/actionTypes/embeddings';
 
 import { runQC } from 'redux/actions/pipeline';
@@ -46,6 +48,7 @@ const initialState = {
       },
     },
   },
+  cellSets: {},
   backendStatus: {
     [experimentId]: {
       status: {
@@ -102,15 +105,15 @@ describe('runQC action', () => {
     expect(actions).toMatchSnapshot();
   });
 
-  it('Runs only the embedding if only changed filter was configureEmbedding', async () => {
+  it('Runs only the embedding if only changed filter was embeddingSettings', async () => {
     fetchMock.resetMocks();
 
     saveProcessingSettings.mockImplementation(() => () => Promise.resolve());
 
-    const onlyConfigureEmbeddingChangedState = _.cloneDeep(initialState);
-    onlyConfigureEmbeddingChangedState.experimentSettings.processing.meta.changedQCFilters = new Set(['configureEmbedding']);
+    const onlyEmbeddingSettingsChangedState = _.cloneDeep(initialState);
+    onlyEmbeddingSettingsChangedState.experimentSettings.processing.meta.changedQCFilters = new Set(['embeddingSettings']);
 
-    const store = mockStore(onlyConfigureEmbeddingChangedState);
+    const store = mockStore(onlyEmbeddingSettingsChangedState);
     await store.dispatch(runQC(experimentId));
 
     await waitForActions(
@@ -125,4 +128,28 @@ describe('runQC action', () => {
 
     expect(actions).toMatchSnapshot();
   });
+
+  it('Runs only clustering if only changed filter was clusteringSettings', async () => {
+    fetchMock.resetMocks();
+
+    saveProcessingSettings.mockImplementation(() => () => Promise.resolve());
+
+    const onlyClusteringSettingsChangedState = _.cloneDeep(initialState);
+    onlyClusteringSettingsChangedState.experimentSettings.processing.meta.changedQCFilters = new Set(['clusteringSettings']);
+
+    const store = mockStore(onlyClusteringSettingsChangedState);
+    await store.dispatch(runQC(experimentId));
+
+    await waitForActions(
+      store,
+      [EXPERIMENT_SETTINGS_DISCARD_CHANGED_QC_FILTERS, CELL_SETS_CLUSTERING_UPDATING],
+    );
+
+    const actions = store.getActions();
+
+    expect(actions[0].type).toEqual(EXPERIMENT_SETTINGS_DISCARD_CHANGED_QC_FILTERS);
+    expect(actions[1].type).toEqual(CELL_SETS_CLUSTERING_UPDATING);
+
+    expect(actions).toMatchSnapshot();
+  });
 });
diff --git a/src/components/data-processing/ConfigureEmbedding/CalculationConfig.jsx b/src/components/data-processing/ConfigureEmbedding/CalculationConfig.jsx
@@ -1,5 +1,5 @@
 import React, {
-  useState, useEffect, useCallback,
+  useState, useEffect,
 } from 'react';
 import _ from 'lodash';
 import { useDispatch, useSelector } from 'react-redux';
@@ -9,9 +9,8 @@ import {
 import PropTypes from 'prop-types';
 import { QuestionCircleOutlined } from '@ant-design/icons';
 
-import { updateFilterSettings, saveProcessingSettings } from 'redux/actions/experimentSettings';
+import { updateFilterSettings } from 'redux/actions/experimentSettings';
 
-import { runCellSetsClustering } from 'redux/actions/cellSets';
 import PreloadContent from '../../PreloadContent';
 
 import SliderWithInput from '../../SliderWithInput';
@@ -32,7 +31,7 @@ const EMBEDD_METHOD_TEXT = 'Reducing the dimensionality does lose some informati
   + 't-SNE and UMAP are stochastic and very much dependent on choice of parameters (t-SNE even more than UMAP) and can yield very different results in different runs. ';
 
 const CalculationConfig = (props) => {
-  const { experimentId, onConfigChange, disabled } = props;
+  const { onConfigChange, disabled } = props;
   const FILTER_UUID = 'configureEmbedding';
   const dispatch = useDispatch();
 
@@ -46,41 +45,19 @@ const CalculationConfig = (props) => {
   const { umap: umapSettings, tsne: tsneSettings } = data?.embeddingSettings.methodSettings || {};
   const { louvain: louvainSettings } = data?.clusteringSettings.methodSettings || {};
 
-  const debouncedClustering = useCallback(
-    _.debounce((resolution) => {
-      dispatch(runCellSetsClustering(experimentId, resolution));
-    }, 1500),
-    [],
-  );
-
-  const [resolution, setResolution] = useState(null);
   const [minDistance, setMinDistance] = useState(null);
 
-  useEffect(() => {
-    if (!resolution && louvainSettings) {
-      setResolution(louvainSettings.resolution);
-    }
-  }, [louvainSettings]);
-
   useEffect(() => {
     if (!minDistance && umapSettings) {
       setMinDistance(umapSettings.minimumDistance);
     }
   }, [umapSettings]);
 
-  const dispatchDebounce = useCallback(_.debounce((f) => {
-    dispatch(f);
-  }, 1500), []);
-
   const updateSettings = (diff) => {
-    if (diff.embeddingSettings) {
-      // If this is an embedding change, indicate to user that their changes are not
-      // applied until they hit Run.
-      onConfigChange();
-    } else {
-      // If it's a clustering change, debounce the save process at 1.5s.
-      dispatchDebounce(saveProcessingSettings(experimentId, FILTER_UUID));
-    }
+    // 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];
+    onConfigChange(settingType);
 
     dispatch(updateFilterSettings(
       FILTER_UUID,
@@ -98,8 +75,6 @@ const CalculationConfig = (props) => {
         },
       },
     });
-
-    onConfigChange();
   };
   const setDistanceMetric = (value) => {
     updateSettings({
@@ -111,8 +86,6 @@ const CalculationConfig = (props) => {
         },
       },
     });
-
-    onConfigChange();
   };
 
   const setLearningRate = (value) => {
@@ -125,8 +98,6 @@ const CalculationConfig = (props) => {
         },
       },
     });
-
-    onConfigChange();
   };
   const setPerplexity = (value) => {
     updateSettings({
@@ -138,8 +109,6 @@ const CalculationConfig = (props) => {
         },
       },
     });
-
-    onConfigChange();
   };
 
   const renderUMAPSettings = () => (
@@ -395,21 +364,14 @@ const CalculationConfig = (props) => {
               max={10}
               step={0.1}
               disabled={disabled}
-              value={resolution}
-              onUpdate={(value) => {
-                if (value === resolution) { return; }
-
-                setResolution(value);
-                updateSettings({
-                  clusteringSettings: {
-                    methodSettings: {
-                      louvain: { resolution: value },
-                    },
+              value={louvainSettings.resolution}
+              onUpdate={(value) => updateSettings({
+                clusteringSettings: {
+                  methodSettings: {
+                    louvain: { resolution: value },
                   },
-                });
-
-                debouncedClustering(value);
-              }}
+                },
+              })}
             />
           </Form.Item>
         </Form>

diff --git a/src/pages/experiments/[experimentId]/data-processing/index.jsx b/src/pages/experiments/[experimentId]/data-processing/index.jsx
@@ -329,7 +329,7 @@ const DataProcessingPage = ({ experimentId, experimentData }) => {
         <ConfigureEmbedding
           experimentId={expId}
           key={key}
-          onConfigChange={() => onConfigChange(key)}
+          onConfigChange={(settingType) => onConfigChange(settingType)}
           stepHadErrors={getStepHadErrors(key)}
         />
       ),
@@ -695,6 +695,10 @@ const DataProcessingPage = ({ experimentId, experimentData }) => {
                   Your navigation within Cellenics will be restricted during this time.
                   Do you want to start?
                 </p>
+                <Alert
+                  message='Note that you will lose all of your annotated cell sets.'
+                  type='warning'
+                />
               </Modal>
             )
         )}

diff --git a/src/redux/actions/pipeline/runQC.js b/src/redux/actions/pipeline/runQC.js
@@ -9,10 +9,10 @@ import {
 import { saveProcessingSettings } from 'redux/actions/experimentSettings';
 import { loadBackendStatus } from 'redux/actions/backendStatus';
 import { loadEmbedding } from 'redux/actions/embedding';
+import { runCellSetsClustering } from 'redux/actions/cellSets';
 
 const runOnlyConfigureEmbedding = async (experimentId, embeddingMethod, dispatch) => {
   await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding'));
-
   dispatch({
     type: EXPERIMENT_SETTINGS_DISCARD_CHANGED_QC_FILTERS,
     payload: {},
@@ -28,6 +28,22 @@ const runOnlyConfigureEmbedding = async (experimentId, embeddingMethod, dispatch
   );
 };
 
+const runOnlyClustering = async (experimentId, resolution, dispatch) => {
+  await dispatch(saveProcessingSettings(experimentId, 'configureEmbedding'));
+  dispatch({
+    type: EXPERIMENT_SETTINGS_DISCARD_CHANGED_QC_FILTERS,
+    payload: {},
+  });
+
+  // Only configure embedding was changed so we run loadEmbedding
+  dispatch(
+    runCellSetsClustering(
+      experimentId,
+      resolution,
+    ),
+  );
+};
+
 // Question for review, I thought of implementing this function for all the URLs here
 // (extracting all the URLs into one single place and using constants to
 //  define which url I am trying to access)
@@ -42,12 +58,26 @@ const runQC = (experimentId) => async (dispatch, getState) => {
   const { processing } = getState().experimentSettings;
   const { changedQCFilters } = processing.meta;
 
-  if (changedQCFilters.size === 1 && changedQCFilters.has('configureEmbedding')) {
-    runOnlyConfigureEmbedding(
-      experimentId,
-      processing.configureEmbedding.embeddingSettings.method,
-      dispatch,
-    );
+  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) {
+    if (embeddingChanged) {
+      runOnlyConfigureEmbedding(
+        experimentId,
+        processing.configureEmbedding.embeddingSettings.method,
+        dispatch,
+      );
+    }
+    if (clusteringChanged) {
+      runOnlyClustering(
+        experimentId,
+        processing.configureEmbedding.clusteringSettings.methodSettings.louvain.resolution,
+        dispatch,
+      );
+    }
 
     return;
   }
@@ -57,7 +87,6 @@ const runQC = (experimentId) => async (dispatch, getState) => {
     const stepConfig = processing[stepKey];
     processingConfigDiff[stepKey] = stepConfig;
   });
-
   try {
     // We are only sending the configuration that we know changed
     // with respect to the one that is already persisted in dynamodb