Fix indexing bug in user-specified colors (#2051)

* repaint category value when color changes

* bug fix incorrect indexing of user colors

* add test for bug 2007

* lint

client/__tests__/util/stateManager/colorHelpers.test.js

@@ -0,0 +1,198 @@
+/* eslint-disable no-bitwise -- unsigned right shift better than Math.round */
+
+/*
+test color helpers
+*/
+import {
+  createColorTable,
+  loadUserColorConfig,
+} from "../../../src/util/stateManager/colorHelpers";
+import * as Dataframe from "../../../src/util/dataframe";
+
+describe("categorical color helpers", () => {
+  /*
+  Primary test constraint for categorical colors is that they are ordered/identified
+  by schema order, NOT by value.  Ie,
+
+    scale(schemaIndex) should match rgb[obsOffset]
+  */
+
+  const schema = indexSchema({
+    annotations: {
+      obs: {
+        columns: [
+          {
+            name: "name_0",
+            type: "string",
+            writable: false,
+          },
+          {
+            name: "continuousColumn",
+            type: "float32",
+            writable: false,
+          },
+          {
+            categories: [
+              "CD4 T cells",
+              "CD14+ Monocytes",
+              "B cells",
+              "CD8 T cells",
+              "NK cells",
+              "FCGR3A+ Monocytes",
+              "Dendritic cells",
+              "Megakaryocytes",
+            ],
+            name: "categoricalColumn",
+            type: "categorical",
+            writable: false,
+          },
+        ],
+        index: "name_0",
+      },
+      var: {
+        columns: [
+          {
+            name: "name_0",
+            type: "string",
+            writable: false,
+          },
+        ],
+        index: "name_0",
+      },
+    },
+    dataframe: {
+      nObs: 2638,
+      nVar: 1838,
+      type: "float32",
+    },
+    layout: {},
+  });
+
+  const catColCategories = schema.annotations.obs.columns[2].categories;
+  const obsDataframe = new Dataframe.Dataframe(
+    [schema.dataframe.nObs, 2],
+    [
+      new Float32Array(schema.dataframe.nObs).map(() => Math.random()),
+      new Array(schema.dataframe.nObs)
+        .fill("")
+        .map(
+          () =>
+            catColCategories[(Math.random() * catColCategories.length) >>> 0]
+        ),
+    ],
+    null,
+    new Dataframe.KeyIndex(["continuousColumn", "categoricalColumn"])
+  );
+
+  test("default category order", () => {
+    const ct = createColorTable(
+      "color by categorical metadata",
+      "categoricalColumn",
+      obsDataframe,
+      schema
+    );
+    expect(ct).toBeDefined();
+    const data = obsDataframe.col("categoricalColumn").asArray();
+    const cats = schema.annotations.obsByName.categoricalColumn.categories;
+    for (let i = 0; i < schema.dataframe.nObs; i += 1) {
+      expect(makeScale(ct.rgb[i])).toEqual(ct.scale(cats.indexOf(data[i])));
+    }
+  });
+
+  test("shuffle category order", () => {
+    const schemaClone = indexSchema(JSON.parse(JSON.stringify(schema)));
+    shuffle(schemaClone.annotations.obsByName.categoricalColumn.categories);
+    const ct = createColorTable(
+      "color by categorical metadata",
+      "categoricalColumn",
+      obsDataframe,
+      schemaClone
+    );
+    expect(ct).toBeDefined();
+    const data = obsDataframe.col("categoricalColumn").asArray();
+    const cats = schemaClone.annotations.obsByName.categoricalColumn.categories;
+    for (let i = 0; i < schemaClone.dataframe.nObs; i += 1) {
+      expect(makeScale(ct.rgb[i])).toEqual(ct.scale(cats.indexOf(data[i])));
+    }
+  });
+
+  test("user defined color order", () => {
+    const cats = schema.annotations.obsByName.categoricalColumn.categories;
+    const shuffleCats = shuffle(
+      Array.from(schema.annotations.obsByName.categoricalColumn.categories)
+    );
+    const userDefinedColorTable = {
+      categoricalColumn: shuffleCats.reduce((acc, label) => {
+        acc[label] = randRGBColor();
+        return acc;
+      }, {}),
+    };
+
+    const userColors = loadUserColorConfig(userDefinedColorTable);
+    expect(userColors).toBeDefined();
+
+    const ct = createColorTable(
+      "color by categorical metadata",
+      "categoricalColumn",
+      obsDataframe,
+      schema,
+      userColors
+    );
+    expect(ct).toBeDefined();
+    const data = obsDataframe.col("categoricalColumn").asArray();
+    for (let i = 0; i < schema.dataframe.nObs; i += 1) {
+      expect(makeScale(ct.rgb[i])).toEqual(
+        ct.scale(cats.indexOf(data[i])).toString()
+      );
+    }
+  });
+});
+
+/*
+TODO:
+1. mix up category order in schema to make sure it works with varied order
+2. user defined colors
+*/
+
+function indexSchema(schema) {
+  schema.annotations.obsByName = Object.fromEntries(
+    schema.annotations?.obs?.columns?.map((v) => [v.name, v]) ?? []
+  );
+  schema.annotations.varByName = Object.fromEntries(
+    schema.annotations?.var?.columns?.map((v) => [v.name, v]) ?? []
+  );
+  schema.layout.obsByName = Object.fromEntries(
+    schema.layout?.obs?.map((v) => [v.name, v]) ?? []
+  );
+  schema.layout.varByName = Object.fromEntries(
+    schema.layout?.var?.map((v) => [v.name, v]) ?? []
+  );
+
+  return schema;
+}
+
+function makeScale(rgb) {
+  // make a scale string from a rgb float triple
+  return `rgb(${(rgb[0] * 255) >>> 0}, ${(rgb[1] * 255) >>> 0}, ${
+    (rgb[2] * 256) >>> 0
+  })`;
+}
+
+function shuffle(array) {
+  for (let i = array.length - 1; i > 0; i -= 1) {
+    const j = (Math.random() * (i + 1)) >>> 0;
+    [array[i], array[j]] = [array[j], array[i]];
+  }
+  return array;
+}
+
+function randHexColor() {
+  const hex = ((Math.random() * 255) >>> 0).toString(16);
+  return `0${hex}`.slice(-2);
+}
+
+function randRGBColor() {
+  return `#${randHexColor()}${randHexColor()}${randHexColor()}`;
+}
+
+/* eslint-enable no-bitwise -- unsigned right shift better than Math.round */

client/src/components/categorical/value/index.js

@@ -174,7 +174,7 @@ class CategoryValue extends React.Component {
     Checks to see if at least one of the following changed:
     * world state
     * the color accessor (what is currently being colored by)
-    * if this catagorical value's selection status has changed
+    * if this categorical value's selection status has changed
     * the crossfilter (ie, global selection state)
 
     If and only if true, update the component
@@ -201,6 +201,13 @@ class CategoryValue extends React.Component {
     const newCount = newCategorySummary.categoryValueCounts[newCategoryIndex];
     const countChanged = count !== newCount;
 
+    // If the user edits an annotation that is currently colored-by, colors may be re-assigned.
+    // This test is conservative - it may cause re-rendering of entire category (all labels)
+    // if any one changes, but only for the currently colored-by category.
+    const colorMightHaveChanged =
+      nextProps.colorAccessor === nextProps.metadataField &&
+      props.categorySummary !== nextProps.categorySummary;
+
     return (
       labelChanged ||
       valueSelectionChange ||
@@ -208,7 +215,8 @@ class CategoryValue extends React.Component {
       annotationsChange ||
       editingLabel ||
       dilationChange ||
-      countChanged
+      countChanged ||
+      colorMightHaveChanged
     );
   };
 

client/src/util/stateManager/colorHelpers.js

@@ -55,8 +55,8 @@ create colors scale and RGB array and return as object. Parameters:
   * userColors - optional user color table
 Returns:
   {
-    scale: color scale
+    scale: function, mapping label index to color scale
-    rgb: cell to color mapping
+    rgb: cell label to color mapping
   }
 */
 function _createColorTable(
@@ -70,7 +70,7 @@ function _createColorTable(
     case "color by categorical metadata": {
       const data = colorByData.col(colorByAccessor).asArray();
       if (userColors && colorByAccessor in userColors) {
-        return createUserColors(data, colorByAccessor, userColors);
+        return createUserColors(data, colorByAccessor, schema, userColors);
       }
       return createColorsByCategoricalMetadata(data, colorByAccessor, schema);
     }
@@ -91,42 +91,41 @@ function _createColorTable(
 }
 export const createColorTable = memoize(_createColorTable);
 
+/**
+ * Create two category label-indexed objects:
+ *    - colors: maps label to RGB triplet for that label (used by graph, etc)
+ *    - scale: function which given label returns d3 color scale for label
+ * Order doesn't matter - everything is keyed by label value.
+ */
 export function loadUserColorConfig(userColors) {
   const convertedUserColors = {};
   Object.keys(userColors).forEach((category) => {
-    // We cannot iterate over keys without sorting
+    const [colors, scaleMap] = Object.keys(userColors[category]).reduce(
-    // because we handle categorical values in alphabetical order __ignoring case__
+      (acc, label) => {
-    //  while Object.keys() _usually_ is ordered alphabetically where all upper characters are less than lowercase (A, B, C, a, b, c)
+        const color = parseRGB(userColors[category][label]);
-    const [colors, scaleMap] = Object.keys(userColors[category])
+        acc[0][label] = color;
-      .sort((a, b) => {
+        acc[1][label] = d3.rgb(255 * color[0], 255 * color[1], 255 * color[2]);
-        a = a.toLowerCase();
+        return acc;
-        b = b.toLowerCase();
+      },
-        if (a === b) return 0;
+      [{}, {}]
-        if (a > b) return 1;
+    );
-        return -1;
-      })
-      .reduce(
-        (acc, label) => {
-          const color = parseRGB(userColors[category][label]);
-          acc[0][label] = color;
-          acc[1][label] = d3.rgb(
-            255 * color[0],
-            255 * color[1],
-            255 * color[2]
-          );
-          return acc;
-        },
-        [{}, {}]
-      );
     const scale = (label) => scaleMap[label];
     convertedUserColors[category] = { colors, scale };
   });
   return convertedUserColors;
 }
 
-function _createUserColors(data, colorAccessor, userColors) {
+function _createUserColors(data, colorAccessor, schema, userColors) {
-  const { colors, scale } = userColors[colorAccessor];
+  const { colors, scale: scaleByLabel } = userColors[colorAccessor];
   const rgb = createRgbArray(data, colors);
+
+  // color scale function param is INDEX (offset) into schema categories. It is NOT label value.
+  // See createColorsByCategoricalMetadata() for another example.
+  const { categories } = schema.annotations.obsByName[colorAccessor];
+  const categoryMap = new Map();
+  categories.forEach((label, idx) => categoryMap.set(idx, label));
+  const scale = (idx) => scaleByLabel(categoryMap.get(idx));
+
   return { rgb, scale };
 }
 const createUserColors = memoize(_createUserColors);