From 2715157793503b2ed6eeef73f949e6b14b66778a Mon Sep 17 00:00:00 2001 From: Bruce Martin Date: Mon, 20 Jul 2020 08:52:41 -0700 Subject: [PATCH] categorical UI improvements (#1649) * return undefined for unknown fields * memoize fetch response * handle rename of user annotation * performance and cleanup * fix snapshots * fix comment * lint --- .../e2e/__snapshots__/e2e.test.js.snap | 2 +- .../__snapshots__/e2eAnnotations.test.js.snap | 8 +- client/src/annoMatrix/annoMatrix.js | 12 +- .../category/categoryFlipperLayout.js | 81 ------- .../components/categorical/category/index.js | 199 +++++++++++++----- .../src/components/categorical/value/index.js | 112 ++++------ client/src/reducers/colors.js | 12 ++ client/src/util/dataframe/cache.js | 26 +++ client/src/util/dataframe/dataframe.js | 16 +- client/src/util/dataframe/index.js | 1 + client/src/util/dataframe/labelIndex.js | 7 + client/src/util/dataframe/util.js | 10 + .../util/stateManager/annotationsHelpers.js | 17 +- 13 files changed, 274 insertions(+), 229 deletions(-) delete mode 100644 client/src/components/categorical/category/categoryFlipperLayout.js create mode 100644 client/src/util/dataframe/cache.js diff --git a/client/__tests__/e2e/__snapshots__/e2e.test.js.snap b/client/__tests__/e2e/__snapshots__/e2e.test.js.snap index a81872c6..fce4a085 100644 --- a/client/__tests__/e2e/__snapshots__/e2e.test.js.snap +++ b/client/__tests__/e2e/__snapshots__/e2e.test.js.snap @@ -2,4 +2,4 @@ exports[`did launch page launched 1`] = `"pbmc3kc3k"`; -exports[`metadata loads categories and values from dataset appear 1`] = `"
louvainvain
tint
"`; +exports[`metadata loads categories and values from dataset appear 1`] = `"
louvainvain
tint
"`; diff --git a/client/__tests__/e2e/__snapshots__/e2eAnnotations.test.js.snap b/client/__tests__/e2e/__snapshots__/e2eAnnotations.test.js.snap index 818426cd..3ad14bca 100644 --- a/client/__tests__/e2e/__snapshots__/e2eAnnotations.test.js.snap +++ b/client/__tests__/e2e/__snapshots__/e2eAnnotations.test.js.snap @@ -2,14 +2,14 @@ exports[`annotations stacked bar graph renders 1`] = ` Array [ - "
TEST-LABELLABEL
0
", - "
unassignedigned
2132
", + "
TEST-LABELLABEL
0
", + "
unassignedigned
2132
", ] `; exports[`annotations stacked bar graph renders 2`] = ` Array [ - "
TEST-LABELLABEL
0
", - "
unassignedigned
2638
", + "
TEST-LABELLABEL
0
", + "
unassignedigned
2638
", ] `; diff --git a/client/src/annoMatrix/annoMatrix.js b/client/src/annoMatrix/annoMatrix.js index 0cb802a8..1b24a6bb 100644 --- a/client/src/annoMatrix/annoMatrix.js +++ b/client/src/annoMatrix/annoMatrix.js @@ -1,4 +1,8 @@ -import { Dataframe, IdentityInt32Index } from "../util/dataframe"; +import { + Dataframe, + IdentityInt32Index, + dataframeMemo, +} from "../util/dataframe"; import { _getColumnDimensionNames, _getColumnSchema, @@ -9,6 +13,8 @@ import { indexEntireSchema } from "../util/stateManager/schemaHelpers"; import { _whereCacheGet, _whereCacheMerge } from "./whereCache"; import _shallowClone from "./clone"; +const _dataframeCache = dataframeMemo(128); + export default class AnnoMatrix { /* Abstract base class for all AnnoMatrix objects. This class provides a proxy @@ -437,7 +443,9 @@ export default class AnnoMatrix { /* everything we need is in the cache, so just cherry-pick requested columns */ const requestedCacheKeys = this._resolveCachedQueries(field, queries); - const response = this._cache[field].subset(null, requestedCacheKeys); + const response = _dataframeCache( + this._cache[field].subset(null, requestedCacheKeys) + ); this._gcUpdateStats(field, response); return response; } diff --git a/client/src/components/categorical/category/categoryFlipperLayout.js b/client/src/components/categorical/category/categoryFlipperLayout.js deleted file mode 100644 index c3efa2bb..00000000 --- a/client/src/components/categorical/category/categoryFlipperLayout.js +++ /dev/null @@ -1,81 +0,0 @@ -import React from "react"; -import { Flipper, Flipped } from "react-flip-toolkit"; - -import * as globals from "../../../globals"; -import Value from "../value"; - -class Category extends React.PureComponent { - renderCategoryItems(optTuples) { - const { - metadataField, - isUserAnno, - categoryData, - categorySummary, - colorAccessor, - colorData, - colorTable, - } = this.props; - - return optTuples.map((tuple, i) => { - return ( - - {(flippedProps) => ( - - )} - - ); - }); - } - - render() { - const { metadataField, categorySummary, children, isExpanded } = this.props; - const { isTruncated } = categorySummary; - const optTuples = [...categorySummary.categoryValueIndices]; - const optTuplesAsKey = optTuples.map((t) => t[0]).join(""); // animation - - return ( -
-
- {children} -
-
- - {isExpanded ? this.renderCategoryItems(optTuples) : null} - -
-
- {isExpanded && isTruncated ? ( -

... truncated list ...

- ) : null} -
-
- ); - } -} - -export default Category; diff --git a/client/src/components/categorical/category/index.js b/client/src/components/categorical/category/index.js index 5761e976..77208b8e 100644 --- a/client/src/components/categorical/category/index.js +++ b/client/src/components/categorical/category/index.js @@ -2,10 +2,11 @@ import React, { useRef, useEffect } from "react"; import { connect, shallowEqual } from "react-redux"; import { FaChevronRight, FaChevronDown } from "react-icons/fa"; import { AnchorButton, Button, Tooltip } from "@blueprintjs/core"; +import { Flipper, Flipped } from "react-flip-toolkit"; import Async from "react-async"; import memoize from "memoize-one"; -import CategoryFlipperLayout from "./categoryFlipperLayout"; +import Value from "../value"; import AnnoMenu from "./annoMenuCategory"; import AnnoDialogEditCategoryName from "./annoDialogEditCategoryName"; import AnnoDialogAddLabel from "./annoDialogAddLabel"; @@ -63,38 +64,16 @@ class Category extends React.PureComponent { return !shallowEqual(props.watchProps, prevProps.watchProps); } - static async fetchData(annoMatrix, metadataField, colors) { - /* - fetch our data and the color-by data if appropriate, and then build a summary - of our category and a color table for the color-by annotation. - */ - const { schema } = annoMatrix; - const { colorAccessor, colorMode } = colors; - let colorDataPromise = Promise.resolve(null); - if (colorAccessor) { - const query = createColorQuery(colorMode, colorAccessor, schema); - if (query) colorDataPromise = annoMatrix.fetch(...query); - } - const [categoryData, colorData] = await Promise.all([ - annoMatrix.fetch("obs", metadataField), - colorDataPromise, - ]); + createCategorySummaryFromDfCol = memoize(createCategorySummaryFromDfCol); - // our data - const column = categoryData.icol(0); - const colSchema = schema.annotations.obsByName[metadataField]; - const categorySummary = createCategorySummaryFromDfCol(column, colSchema); - return [categoryData, categorySummary, colorData]; - } - - getSelectionState = memoize((categorySummary) => { + getSelectionState(categorySummary) { const { categoricalSelection, metadataField } = this.props; return Category.getSelectionState( categoricalSelection, metadataField, categorySummary ); - }); + } handleColorChange = () => { const { dispatch, metadataField } = this.props; @@ -133,7 +112,7 @@ class Category extends React.PureComponent { const { annoMatrix, metadataField, colors } = props.watchProps; const { crossfilter } = this.props; - const [categoryData, categorySummary, colorData] = await Category.fetchData( + const [categoryData, categorySummary, colorData] = await this.fetchData( annoMatrix, metadataField, colors @@ -150,6 +129,33 @@ class Category extends React.PureComponent { }; }; + async fetchData(annoMatrix, metadataField, colors) { + /* + fetch our data and the color-by data if appropriate, and then build a summary + of our category and a color table for the color-by annotation. + */ + const { schema } = annoMatrix; + const { colorAccessor, colorMode } = colors; + let colorDataPromise = Promise.resolve(null); + if (colorAccessor) { + const query = createColorQuery(colorMode, colorAccessor, schema); + if (query) colorDataPromise = annoMatrix.fetch(...query); + } + const [categoryData, colorData] = await Promise.all([ + annoMatrix.fetch("obs", metadataField), + colorDataPromise, + ]); + + // our data + const column = categoryData.icol(0); + const colSchema = schema.annotations.obsByName[metadataField]; + const categorySummary = this.createCategorySummaryFromDfCol( + column, + colSchema + ); + return [categoryData, categorySummary, colorData]; + } + updateColorTable(colorData) { // color table, which may be null const { schema, colors, metadataField } = this.props; @@ -228,7 +234,7 @@ class Category extends React.PureComponent { )} - + {(asyncProps) => { const { colorAccessor, @@ -239,15 +245,17 @@ class Category extends React.PureComponent { isColorAccessor, handleCategoryToggleAllClick, } = asyncProps; + const isTruncated = !!categorySummary?.isTruncated; + const selectionState = this.getSelectionState(categorySummary); return ( - - +
+ +
+
+ { + /* values*/ + isExpanded ? ( + + ) : null + } +
+
+ {isExpanded && isTruncated ? ( +

... truncated list ...

+ ) : null} +
+ + ); + } +); + +const CategoryValueList = React.memo( + ({ + isUserAnno, + metadataField, + categoryData, + categorySummary, + colorAccessor, + colorData, + colorTable, + }) => { + const tuples = [...categorySummary.categoryValueIndices]; + + /* + Render the value list. If this is a user annotation, we use a flipper + animation, if read-only, we don't bother and save a few bits of perf. + */ + if (!isUserAnno) { + return ( + <> + {tuples.map(([value, index]) => ( + + ))} + + ); + } + + /* User annotation */ + const flipKey = tuples.map((t) => t[0]).join(""); + return ( + + {tuples.map(([value, index]) => ( + + + + ))} + ); } ); diff --git a/client/src/components/categorical/value/index.js b/client/src/components/categorical/value/index.js index aa088716..f40a9a99 100644 --- a/client/src/components/categorical/value/index.js +++ b/client/src/components/categorical/value/index.js @@ -29,24 +29,30 @@ const CHART_WIDTH = 100; /* this is defined outside of the class so we can use it in connect() */ function _currentLabelAsString(ownProps) { - const { categorySummary, categoryIndex } = ownProps; + const { label } = ownProps; // when called as a function, the String() constructor performs type conversion, // and returns a primitive string. - return String(categorySummary.categoryValues[categoryIndex]); + return String(label); } @connect((state, ownProps) => { const { pointDilation, categoricalSelection } = state; - const { metadataField } = ownProps; + const { metadataField, categorySummary, categoryIndex } = ownProps; const isDilated = pointDilation.metadataField === metadataField && pointDilation.categoryField === _currentLabelAsString(ownProps); + + const category = categoricalSelection[metadataField]; + const label = categorySummary.categoryValues[categoryIndex]; + const isSelected = category.get(label) ?? true; + return { - categoricalSelection, annotations: state.annotations, schema: state.annoMatrix?.schema, ontology: state.ontology, isDilated, + isSelected, + label, }; }) class CategoryValue extends React.Component { @@ -58,14 +64,8 @@ class CategoryValue extends React.Component { } componentDidUpdate(prevProps) { - const { - categoricalSelection, - metadataField, - categoryIndex, - categorySummary, - } = this.props; + const { metadataField, categoryIndex, categorySummary } = this.props; if ( - prevProps.categoricalSelection !== categoricalSelection || prevProps.metadataField !== metadataField || prevProps.categoryIndex !== categoryIndex || prevProps.categorySummary !== categorySummary @@ -84,28 +84,19 @@ class CategoryValue extends React.Component { return colorAccessor && !isColorBy && !annotations.isEditingLabelName; } - getLabel() { - const { categoryIndex, categorySummary } = this.props; - const label = categorySummary.categoryValues[categoryIndex]; - return label; - } - handleDeleteValue = () => { - const { dispatch, metadataField } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, label } = this.props; dispatch(actions.annotationDeleteLabelFromCategory(metadataField, label)); }; handleAddCurrentSelectionToThisLabel = () => { - const { dispatch, metadataField } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, label } = this.props; dispatch(actions.annotationLabelCurrentSelection(metadataField, label)); }; handleEditValue = (e) => { - const { dispatch, metadataField } = this.props; + const { dispatch, metadataField, label } = this.props; const { editedLabelText } = this.state; - const label = this.getLabel(); this.cancelEditMode(); dispatch( actions.annotationRenameLabelInCategory( @@ -118,8 +109,7 @@ class CategoryValue extends React.Component { }; handleCreateArbitraryLabel = (txt) => { - const { dispatch, metadataField } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, label } = this.props; this.cancelEditMode(); dispatch( actions.annotationRenameLabelInCategory(metadataField, label, txt) @@ -137,8 +127,7 @@ class CategoryValue extends React.Component { }; activateEditLabelMode = () => { - const { dispatch, metadataField, categoryIndex } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, categoryIndex, label } = this.props; dispatch({ type: "annotation: activate edit label mode", metadataField, @@ -148,8 +137,7 @@ class CategoryValue extends React.Component { }; cancelEditMode = () => { - const { dispatch, metadataField, categoryIndex } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, categoryIndex, label } = this.props; this.setState({ editedLabelText: this.currentLabelAsString(), }); @@ -191,24 +179,17 @@ class CategoryValue extends React.Component { If and only if true, update the component */ const { props, state } = this; - const { - metadataField, - categoryIndex, - categoricalSelection, - categorySummary, - } = props; + const { categoryIndex, categorySummary, isSelected } = props; const { categoryIndex: newCategoryIndex, - categoricalSelection: newCategoricalSelection, categorySummary: newCategorySummary, + isSelected: newIsSelected, } = nextProps; const label = categorySummary.categoryValues[categoryIndex]; const newLabel = newCategorySummary.categoryValues[newCategoryIndex]; const labelChanged = label !== newLabel; - const valueSelectionChange = - categoricalSelection[metadataField].get(label) !== - newCategoricalSelection[metadataField].get(newLabel); + const valueSelectionChange = isSelected !== newIsSelected; const colorAccessorChange = props.colorAccessor !== nextProps.colorAccessor; const annotationsChange = props.annotations !== nextProps.annotations; @@ -250,8 +231,7 @@ class CategoryValue extends React.Component { }; handleMouseEnter = () => { - const { dispatch, metadataField, categoryIndex } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, categoryIndex, label } = this.props; dispatch({ type: "category value mouse hover start", metadataField, @@ -261,8 +241,7 @@ class CategoryValue extends React.Component { }; handleMouseExit = () => { - const { dispatch, metadataField, categoryIndex } = this.props; - const label = this.getLabel(); + const { dispatch, metadataField, categoryIndex, label } = this.props; dispatch({ type: "category value mouse hover end", metadataField, @@ -393,21 +372,21 @@ class CategoryValue extends React.Component { return false; } - renderMiniStackedBar = (categoryValue) => { + renderMiniStackedBar = () => { const { - categoricalSelection, colorAccessor, metadataField, categoryData, colorData, colorTable, schema, + label, } = this.props; const isColorBy = metadataField === colorAccessor; if ( !this.shouldRenderStackedBarOrHistogram || - !categoricalSelection[colorAccessor] || + !AnnotationsHelpers.isCategoricalAnnotation(schema, colorAccessor) || isColorBy ) { return null; @@ -419,7 +398,7 @@ class CategoryValue extends React.Component { categoryData, colorAccessor, colorData, - categoryValue, + label, colorTable, schema, CHART_WIDTH @@ -446,20 +425,21 @@ class CategoryValue extends React.Component { ); }; - renderMiniHistogram = (categoryValue) => { + renderMiniHistogram = () => { const { - categoricalSelection, colorAccessor, metadataField, colorData, categoryData, colorTable, + schema, + label, } = this.props; const colorScale = colorTable?.scale; if ( !this.shouldRenderStackedBarOrHistogram || - categoricalSelection[colorAccessor] + !AnnotationsHelpers.isContinuousAnnotation(schema, colorAccessor) ) { return null; } @@ -470,7 +450,7 @@ class CategoryValue extends React.Component { categoryData, colorAccessor, colorData, - categoryValue, + label, CHART_WIDTH, VALUE_HEIGHT ) ?? {}; @@ -486,7 +466,7 @@ class CategoryValue extends React.Component { }} /* eslint-enable react/jsx-props-no-spreading -- enable */ obsOrVarContinuousFieldDisplayName={colorAccessor} - domainLabel={categoryValue} + domainLabel={label} height={VALUE_HEIGHT} width={CHART_WIDTH} /> @@ -495,32 +475,24 @@ class CategoryValue extends React.Component { render() { const { - categoricalSelection, metadataField, categoryIndex, colorAccessor, colorTable, - i, isUserAnno, annotations, ontology, - // flippedProps is potentially brittle, their docs want {...flippedProps} on our div, - // our lint doesn't like jsx spread, we are version pinned to prevent api change on their part - flippedProps, isDilated, + isSelected, categorySummary, + label, } = this.props; const colorScale = colorTable?.scale; const ontologyEnabled = ontology?.enabled ?? false; const { editedLabelText } = this.state; - if (!categoricalSelection) return null; - - const category = categoricalSelection[metadataField]; - const selected = category.get(this.getLabel()) ?? true; const count = categorySummary.categoryValueCounts[categoryIndex]; - const value = categorySummary.categoryValues[categoryIndex]; const displayString = this.currentLabelAsString(); /* this is the color scale, so add swatches below */ @@ -559,10 +531,6 @@ class CategoryValue extends React.Component { return (
- {this.renderMiniStackedBar(value)} - {this.renderMiniHistogram(value)} + {this.renderMiniStackedBar()} + {this.renderMiniHistogram()}
@@ -704,7 +672,7 @@ class CategoryValue extends React.Component { height: VALUE_HEIGHT, backgroundColor: isColorBy && categoryValueIndices - ? colorScale(categoryValueIndices.get(value)) + ? colorScale(categoryValueIndices.get(label)) : "inherit", }} /> @@ -745,7 +713,7 @@ class CategoryValue extends React.Component { disabled={this.isAddCurrentSelectionDisabled( crossfilter, metadataField, - value + label )} /> )} diff --git a/client/src/reducers/colors.js b/client/src/reducers/colors.js index 9cce683a..a95b51ce 100644 --- a/client/src/reducers/colors.js +++ b/client/src/reducers/colors.js @@ -33,6 +33,18 @@ const ColorsReducer = ( return state; } + case "annotation: category edited": { + const { colorAccessor } = state; + if (action.metadataField !== colorAccessor) { + return state; + } + /* else update colorAccessor */ + return { + ...state, + colorAccessor: action.newCategoryText, + }; + } + case "annotation: delete category": { const { colorAccessor } = state; if (action.metadataField !== colorAccessor) { diff --git a/client/src/util/dataframe/cache.js b/client/src/util/dataframe/cache.js new file mode 100644 index 00000000..bfffeef3 --- /dev/null +++ b/client/src/util/dataframe/cache.js @@ -0,0 +1,26 @@ +/* + +DataframeCache - to improve memoization, we often want to have the same +Dataframe object returned when the contents of the dataframe are identical. +The core dataframe class does not support this, but it does attempt to maintain +strict eq and immutability of columns indices. + +This is a function that will maintain a least-recently created cache of Dataframe +objects. +*/ + +import { memoize } from "./util"; + +function hashDataframe(df) { + if (df.isEmpty()) return ""; + return df.__columnsAccessor.map((c) => c.__id).join(","); +} + +function noop(df) { + return df; +} + +const dataframeMemo = (capacity = 100) => + memoize(noop, hashDataframe, capacity); + +export default dataframeMemo; diff --git a/client/src/util/dataframe/dataframe.js b/client/src/util/dataframe/dataframe.js index beec72a0..98693bd0 100644 --- a/client/src/util/dataframe/dataframe.js +++ b/client/src/util/dataframe/dataframe.js @@ -5,6 +5,7 @@ import { isArrayOrTypedArray, callOnceLazy, memoize, + __getMemoId, } from "./util"; import { summarizeContinuous, @@ -72,17 +73,6 @@ Dataframe **/ class Dataframe { - /** - memoization helpers. - **/ - static __DataframeId__ = 0; - - static __getId() { - const id = Dataframe.__DataframeId__; - Dataframe.__DataframeId__ += 1; - return id; - } - /** Constructors & factories **/ @@ -126,7 +116,7 @@ class Dataframe { this.length = nRows; // convenience accessor for row dimension this.rowIndex = rowIndex; this.colIndex = colIndex; - this.__id = Dataframe.__getId(); + this.__id = __getMemoId(); this.__compile(__columnsAccessor); Object.freeze(this); @@ -202,7 +192,7 @@ class Dataframe { */ const { length } = column; - const __id = Dataframe.__getId(); + const __id = __getMemoId(); /* get value by row label */ const get = function get(rlabel) { diff --git a/client/src/util/dataframe/index.js b/client/src/util/dataframe/index.js index 15d95c1b..0432a6e5 100644 --- a/client/src/util/dataframe/index.js +++ b/client/src/util/dataframe/index.js @@ -5,3 +5,4 @@ export { KeyIndex, isLabelIndex, } from "./labelIndex"; +export { default as dataframeMemo } from "./cache"; diff --git a/client/src/util/dataframe/labelIndex.js b/client/src/util/dataframe/labelIndex.js index b88bb8ed..fe8a48fe 100644 --- a/client/src/util/dataframe/labelIndex.js +++ b/client/src/util/dataframe/labelIndex.js @@ -5,6 +5,7 @@ for how this is used. **/ import { rangeFill as fillRange } from "../range"; +import { __getMemoId } from "./util"; /* Private utility functions @@ -32,6 +33,10 @@ class IdentityInt32Index { this.maxOffset = maxOffset; } + get __id() { + return `IdentityInt32Index_${this.maxOffset}`; + } + labels() { // memoize const k = fillRange(new Int32Array(this.maxOffset)); @@ -162,6 +167,7 @@ class DenseInt32Index { this.minLabel = minLabel; this.rindex = labels; this.index = index; + this.__id = __getMemoId(); this.__compile(); } @@ -288,6 +294,7 @@ class KeyIndex { this.index = index; this.rindex = rindex; + this.__id = __getMemoId(); this.__compile(); } diff --git a/client/src/util/dataframe/util.js b/client/src/util/dataframe/util.js index 09be54af..aa00e0c8 100644 --- a/client/src/util/dataframe/util.js +++ b/client/src/util/dataframe/util.js @@ -52,3 +52,13 @@ export function memoize(fn, hashFn, maxResultsCached = -1) { return wrap; } + +/** +memoization helpers - just a global counter. +**/ +let __DataframeMemoId__ = 0; +export function __getMemoId() { + const id = __DataframeMemoId__; + __DataframeMemoId__ += 1; + return id; +} diff --git a/client/src/util/stateManager/annotationsHelpers.js b/client/src/util/stateManager/annotationsHelpers.js index 06f8d74a..999ee961 100644 --- a/client/src/util/stateManager/annotationsHelpers.js +++ b/client/src/util/stateManager/annotationsHelpers.js @@ -18,13 +18,24 @@ categorical annotations to be writable. */ export function isCategoricalAnnotation(schema, name) { - /* we treat any string, categorical or boolean as a categorical */ - const { type } = schema.annotations.obsByName[name]; + /* + we treat any string, categorical or boolean as a categorical. + Return true/false/undefined (for unkonwn fields) + */ + const colSchema = schema.annotations.obsByName[name]; + if (colSchema === undefined) return undefined; + const { type } = colSchema; return type === "string" || type === "boolean" || type === "categorical"; } export function isContinuousAnnotation(schema, name) { - return !isCategoricalAnnotation(schema, name); + /* + Return true/false/undefined + */ + const colSchema = schema.annotations.obsByName[name]; + if (colSchema === undefined) return undefined; + const { type } = colSchema; + return !(type === "string" || type === "boolean" || type === "categorical"); } function _isUserAnnotation(schema, name) {