Fix label sorting bugs (#1102)

* move category label sort to utils

* refactor cat label sort

* fix category label sort and color assignment

* convert whitespace from tabs to spaces

client/src/components/categorical/category.js

@@ -20,7 +20,7 @@ import {
 
 import * as globals from "../../globals";
 import Value from "./value";
-import sortedCategoryValues from "./util";
+import sortedCategoryLabels from "../../util/catLabelSort";
 import { AnnotationsHelpers } from "../../util/stateManager";
 
 @connect(state => ({
@@ -321,11 +321,8 @@ class Category extends React.Component {
       annotations
     } = this.props;
     const { isTruncated } = categoricalSelection[metadataField];
-
     const cat = categoricalSelection[metadataField];
-    const optTuples = sortedCategoryValues(isUserAnno, [
+    const optTuples = [...cat.categoryValueIndices];
-      ...cat.categoryValueIndices
-    ]);
     const optTuplesAsKey = _.map(optTuples, t => t[0]).join(""); // animation
     const allCategoryNames = _.keys(categoricalSelection);
 

client/src/components/categorical/util.js

@@ -1,47 +0,0 @@
-// jshint esversion: 6
-
-// values is [ [optVal, optIdx], ...]
-// index is range array
-// return sorted index
-
-/*
-Sort category values (labels) in the order we want for presentation.
-
-TL;DR: numeric sort for number-like strings, then strings in case-ignoring alpha
-order.  Except, when isUserAnno is true, pin globals.unassignedCategoryLabel 
-to the end.
-
-*/
-
-import isNumber from "is-number";
-import * as globals from "../../globals";
-
-const sortedCategoryValues = (isUserAnno, values) => {
-  /* this sort could be memoized for perf */
-
-  const strings = [];
-  const ints = [];
-  const unassigned = [];
-
-  values.forEach(v => {
-    if (isUserAnno && v[0] === globals.unassignedCategoryLabel) {
-      unassigned.push(v);
-    } else if (isNumber(v[0])) {
-      ints.push(v);
-    } else {
-      strings.push(v);
-    }
-  });
-
-  strings.sort((a, b) => {
-    const textA = String(a[0]).toUpperCase();
-    const textB = String(b[0]).toUpperCase();
-    return textA < textB ? -1 : textA > textB ? 1 : 0;
-  });
-
-  ints.sort((a, b) => +a[0] - +b[0]);
-
-  return ints.concat(strings, unassigned);
-};
-
-export default sortedCategoryValues;

client/src/util/catLabelSort.js

@@ -0,0 +1,47 @@
+/*
+Sort category values (labels) in the order we want for presentation.
+
+TL;DR: sort order is:
+* numbers or number-like strings first, in numeric order
+* most strings, in case-insenstive unicode sort order
+* then 'nan' (any case)
+* then, IF isUseAnno is true, globals.unassignedCategoryLabel
+*/
+
+import isNumber from "is-number";
+import * as globals from "../globals";
+import { memoize } from "./dataframe/util";
+
+function caseInsensitiveCompare(a, b) {
+  const textA = String(a).toUpperCase();
+  const textB = String(b).toUpperCase();
+  return textA < textB ? -1 : textA > textB ? 1 : 0;
+}
+
+const catLabelSort = (isUserAnno, values) => {
+  /* this sort could be memoized for perf */
+
+  const strings = [];
+  const ints = [];
+  const unassignedOrNaN = [];
+
+  values.forEach(v => {
+    if (isUserAnno && v === globals.unassignedCategoryLabel) {
+      unassignedOrNaN.push(v);
+    } else if (String(v).toLowerCase() === "nan") {
+      unassignedOrNaN.push(v);
+    } else if (isNumber(v)) {
+      ints.push(v);
+    } else {
+      strings.push(v);
+    }
+  });
+
+  strings.sort(caseInsensitiveCompare);
+  ints.sort((a, b) => +a - +b);
+  unassignedOrNaN.sort(caseInsensitiveCompare);
+
+  return ints.concat(strings, unassignedOrNaN);
+};
+
+export default catLabelSort;

client/src/util/stateManager/controlsHelpers.js

@@ -38,18 +38,28 @@ Remember that option values can be ANY js type, except undefined/null.
   }
 */
 function topNCategories(colSchema, summary, N) {
+  /* return top N by occurance in the data, preserving original category order */
   const { categories } = colSchema;
-  const counts = _.map(categories, cat => summary.categoryCounts.get(cat) ?? 0);
+  const counts = categories.map(cat => summary.categoryCounts.get(cat) ?? 0);
+
+  if (categories.length <= N) {
+    return [categories, counts];
+  }
+
   const sortIndex = fillRange(new Array(categories.length)).sort(
     (a, b) => counts[b] - counts[a]
   );
-  const sortedCategories = _.map(sortIndex, i => categories[i]);
+  const topNindices = new Set(sortIndex.slice(0, N));
-  const sortedCounts = _.map(sortIndex, i => counts[i]);
 
-  if (sortedCategories.length < N) {
+  const topNCategories = [];
-    return [sortedCategories, sortedCounts];
+  const topNCounts = [];
+  for (let i = 0; i < categories.length; i += 1) {
+    if (topNindices.has(i)) {
+      topNCategories.push(categories[i]);
+      topNCounts.push(counts[i]);
+    }
   }
-  return [sortedCategories.slice(0, N), sortedCounts.slice(0, N)];
+  return [topNCategories, topNCounts];
 }
 
 export function selectableCategoryNames(world, maxCategoryItems) {

client/src/util/stateManager/schemaHelpers.js

@@ -4,93 +4,107 @@ Helpers for schema management
 import _ from "lodash";
 
 import fromEntries from "../fromEntries";
+import catLabelSort from "../catLabelSort";
 
 /*
 System wide schema assumptions:
-	- schema and data wil be consistent (eg, for user-created annotations)
+  - schema and data wil be consistent (eg, for user-created annotations)
-	- schema will be internally self-consistent (eg, index matches columns)
+  - schema will be internally self-consistent (eg, index matches columns)
-	- world & universe schema are same - only data is subset
+  - world & universe schema are same - only data is subset
 */
 
 export function indexEntireSchema(schema) {
-	/* Index schema for ease of use */
+  /* Index schema for ease of use */
-	schema.annotations.obsByName = fromEntries(
+  schema.annotations.obsByName = fromEntries(
-		schema.annotations.obs.columns.map(v => [v.name, v])
+    schema.annotations.obs.columns.map(v => [v.name, v])
-	);
+  );
-	schema.annotations.varByName = fromEntries(
+  schema.annotations.varByName = fromEntries(
-		schema.annotations.var.columns.map(v => [v.name, v])
+    schema.annotations.var.columns.map(v => [v.name, v])
-	);
+  );
-	schema.layout.obsByName = fromEntries(
+  schema.layout.obsByName = fromEntries(
-		schema.layout.obs.map(v => [v.name, v])
+    schema.layout.obs.map(v => [v.name, v])
-	);
+  );
-	schema.layout.varByName = fromEntries(
+  schema.layout.varByName = fromEntries(
-		schema.layout.var.map(v => [v.name, v])
+    schema.layout.var.map(v => [v.name, v])
-	);
+  );
 
-	return schema;
+  return schema;
+}
+
+export function sortAllCategorical(schema) {
+  /* UI relies on OBS annotation categories being in presentation sort order */
+  schema.annotations.obs.columns.forEach(c => {
+    if (c.categories) {
+      c.categories = catLabelSort(c.writable, c.categories);
+    }
+  });
 }
 
 function _copy(schema) {
-	/* redux copy conventions - WARNING, only for modifyign obs annotations */
+  /* redux copy conventions - WARNING, only for modifyign obs annotations */
-	return {
+  return {
-		...schema,
+    ...schema,
-		annotations: {
+    annotations: {
-			...schema.annotations,
+      ...schema.annotations,
-			obs: _.cloneDeep(schema.annotations.obs)
+      obs: _.cloneDeep(schema.annotations.obs)
-		}
+    }
-	};
+  };
 }
 
 function _reindex(schema) {
-	/* reindex obs annotations ONLY */
+  /* reindex obs annotations ONLY */
-	schema.annotations.obsByName = fromEntries(
+  schema.annotations.obsByName = fromEntries(
-		schema.annotations.obs.columns.map(v => [v.name, v])
+    schema.annotations.obs.columns.map(v => [v.name, v])
-	);
+  );
-	return schema;
+  return schema;
 }
 
 export function removeObsAnnoColumn(schema, name) {
-	const newSchema = _copy(schema);
+  const newSchema = _copy(schema);
-	newSchema.annotations.obs.columns = schema.annotations.obs.columns.filter(
+  newSchema.annotations.obs.columns = schema.annotations.obs.columns.filter(
-		v => v.name !== name
+    v => v.name !== name
-	);
+  );
-	return _reindex(newSchema);
+  return _reindex(newSchema);
 }
 
 export function addObsAnnoColumn(schema, name, defn) {
-	const newSchema = _copy(schema);
+  const newSchema = _copy(schema);
-	newSchema.annotations.obs.columns.push(defn);
+  newSchema.annotations.obs.columns.push(defn);
-	return _reindex(newSchema);
+  return _reindex(newSchema);
 }
 
 export function removeObsAnnoCategory(schema, name, category) {
-	/* remove a category from a categorical annotation */
+  /* remove a category from a categorical annotation */
-	const categories = schema.annotations.obsByName[name]?.categories;
+  const categories = schema.annotations.obsByName[name]?.categories;
-	if (!categories)
+  if (!categories)
-		throw new Error("column does not exist or is not categorical");
+    throw new Error("column does not exist or is not categorical");
 
-	const idx = categories.indexOf(category);
+  const idx = categories.indexOf(category);
-	if (idx === -1) throw new Error("category does not exist");
+  if (idx === -1) throw new Error("category does not exist");
 
-	const newSchema = _reindex(_copy(schema));
+  const newSchema = _reindex(_copy(schema));
 
-	/* remove category */
+  /* remove category.  Do not need to resort as this can't change presentation order */
-	newSchema.annotations.obsByName[name].categories.splice(idx, 1);
+  newSchema.annotations.obsByName[name].categories.splice(idx, 1);
-	return newSchema;
+  return newSchema;
 }
 
 export function addObsAnnoCategory(schema, name, category) {
-	/* add a category to a categorical annotation */
+  /* add a category to a categorical annotation */
-	const categories = schema.annotations.obsByName[name]?.categories;
+  const categories = schema.annotations.obsByName[name]?.categories;
-	if (!categories)
+  if (!categories)
-		throw new Error("column does not exist or is not categorical");
+    throw new Error("column does not exist or is not categorical");
 
-	const idx = categories.indexOf(category);
+  const idx = categories.indexOf(category);
-	if (idx !== -1) throw new Error("category already exists");
+  if (idx !== -1) throw new Error("category already exists");
 
-	const newSchema = _reindex(_copy(schema));
+  const newSchema = _reindex(_copy(schema));
 
-	/* remove category */
+  /* add category, retaining presentation sort order */
-	newSchema.annotations.obsByName[name].categories.push(category);
+  const catAnno = newSchema.annotations.obsByName[name];
-	return newSchema;
+  catAnno.categories = catLabelSort(catAnno.writable, [
+    ...catAnno.categories,
+    category
+  ]);
+  return newSchema;
 }

client/src/util/stateManager/universe.js

@@ -4,7 +4,7 @@ import { unassignedCategoryLabel } from "../../globals";
 import { decodeMatrixFBS } from "./matrix";
 import * as Dataframe from "../dataframe";
 import { isFpTypedArray } from "../typeHelpers";
-import { indexEntireSchema } from "./schemaHelpers";
+import { indexEntireSchema, sortAllCategorical } from "./schemaHelpers";
 import { isCategoricalAnnotation } from "./annotationsHelpers";
 
 /*
@@ -187,6 +187,7 @@ export function createUniverseFromResponse(
   }
 
   reconcileSchemaCategoriesWithSummary(universe);
+  sortAllCategorical(universe.schema);
   indexEntireSchema(universe.schema);
 
   /* sanity checks */
@@ -196,7 +197,7 @@ export function createUniverseFromResponse(
     )
   ) {
     throw new Error(
-      "Writable continuous obs annotations are not supproted - failed to laod"
+      "Writable continuous obs annotations are not supported - failed to load"
     );
   }