From 93920d9a727e7fc23fcad81fdcb90c93018c7a43 Mon Sep 17 00:00:00 2001 From: bkmartinjr Date: Wed, 9 May 2018 08:13:37 -0700 Subject: [PATCH 1/2] performance - graph, scatterplot and brush select --- src/actions/index.js | 28 +++++ src/components/graph/graph.js | 88 +++++---------- src/components/scatterplot/scatterplot.js | 105 ++++++++---------- src/globals.js | 26 ++--- src/middleware/updateCellColors.js | 9 +- .../updateCellSelectionMiddleware.js | 36 +++--- src/reducers/controls.js | 33 ++++-- src/util/scaleLinear.js | 18 +++ 8 files changed, 178 insertions(+), 165 deletions(-) create mode 100644 src/util/scaleLinear.js diff --git a/src/actions/index.js b/src/actions/index.js index 45118e80..9bccf0db 100644 --- a/src/actions/index.js +++ b/src/actions/index.js @@ -1,5 +1,6 @@ // jshint esversion: 6 import * as globals from "../globals"; +import store from "../reducers"; import URI from "urijs"; import _ from "lodash"; @@ -81,6 +82,30 @@ const initialize = () => { }; }; +// This code defends against the case where /expression returns a cellname +// never seen before (ie, not returned by /cells). This should not happen +// (see https://github.com/chanzuckerberg/cellxgene-rest-api/issues/34) but +// occasionally does. +// +function cleanupExpressionResponse(data) { + const s = store.getState(); + const metadata = s.controls.currentCellSelectionMap; + let errorOccured = false; + const newcells = _.filter(data.data.cells, cell => { + const found = metadata[cell.cellname]; + errorOccured = errorOccured || !found; + return found; + }); + + if (errorOccured) { + console.error( + "Warning: /expression REST API returned unexpected cell names -- discarding surprises." + ); + data.data.cells = newcells; + } + return data; +} + const requestGeneExpressionCounts = () => { return (dispatch, getState) => { dispatch({ type: "get expression started" }); @@ -91,6 +116,7 @@ const requestGeneExpressionCounts = () => { }) }) .then(res => res.json()) + .then(data => cleanupExpressionResponse(data)) .then( data => dispatch({ type: "get expression success", data }), error => dispatch({ type: "get expression error", error }) @@ -112,6 +138,7 @@ const requestSingleGeneExpressionCountsForColoringPOST = gene => { }) }) .then(res => res.json()) + .then(data => cleanupExpressionResponse(data)) .then( data => dispatch({ @@ -142,6 +169,7 @@ const requestGeneExpressionCountsPOST = genes => { }) }) .then(res => res.json()) + .then(data => cleanupExpressionResponse(data)) .then( data => dispatch({ type: "get expression success", data }), error => dispatch({ type: "get expression error", error }) diff --git a/src/components/graph/graph.js b/src/components/graph/graph.js index c1691953..39ba107f 100644 --- a/src/components/graph/graph.js +++ b/src/components/graph/graph.js @@ -13,6 +13,7 @@ import fit from "canvas-fit"; import _camera from "../../util/camera.js"; import _regl from "regl"; import _drawPoints from "./drawPointsRegl"; +import { scaleLinear } from "../../util/scaleLinear"; import FaCrosshair from "react-icons/lib/fa/crosshairs"; import FaZoom from "react-icons/lib/fa/search-plus"; @@ -41,7 +42,7 @@ import FaSave from "react-icons/lib/fa/download"; colorAccessor: state.controls.colorAccessor, colorScale: state.controls.colorScale, continuousSelection: state.controls.continuousSelection, - graphMap: state.controls.graphMap, + graphVec: state.controls.graphVec, currentCellSelection: state.controls.currentCellSelection, graphBrushSelection: state.controls.graphBrushSelection, opacityForDeselectedCells: state.controls.opacityForDeselectedCells @@ -103,72 +104,43 @@ class Graph extends React.Component { sizeBuffer }); } + componentWillReceiveProps(nextProps) { - /* maybe should do a check here to confirm ref exists and pass it? */ - // if ( - // this.state.ctx && - // nextProps.vertices - // // nextProps.expressions && - // // nextProps.expressionsCountsMap && - // ) { - // drawGraphUsingRenderQueue( - // this.state.ctx, - // nextProps.expressionsCountsMap, - // nextProps.colorAccessor, - // nextProps.ranges, /* assumption that this exists if vertices does both are on cells */ - // nextProps.metadata, - // nextProps.currentCellSelection, - // nextProps.graphBrushSelection, - // nextProps.colorScale, - // nextProps.graphMap, - // nextProps.opacityForDeselectedCells, - // ) - // } if (this.state.regl && nextProps.vertices) { - const _currentCellSelectionMap = _.keyBy( - nextProps.currentCellSelection, - "CellName" - ); /* move me to the reducer */ + const vertices = nextProps.currentCellSelection; + const vertexCount = vertices.length; + const positions = new Float32Array(2 * vertexCount); + const colors = new Float32Array(3 * vertexCount); + const sizes = new Float32Array(vertexCount); - const positions = []; - positions.length = nextProps.currentCellSelection.length; - const colors = []; - colors.length = nextProps.currentCellSelection.length; - const sizes = []; - sizes.length = nextProps.currentCellSelection.length; - - const glScaleX = d3 - .scaleLinear() - .domain([0, 1]) - .range([-1, 1]); /* padding */ - - const glScaleY = d3 - .scaleLinear() - .domain([0, 1]) - .range([1, -1]); /* padding */ + // d3.scaleLinear().domain([0,1]).range([-1,1]) + const glScaleX = scaleLinear([0, 1], [-1, 1]); + // d3.scaleLinear().domain([0,1]).range([1,-1]) + const glScaleY = scaleLinear([0, 1], [1, -1]); /* Construct Vectors */ - _.each(nextProps.currentCellSelection, (cell, i) => { - if (nextProps.graphMap[cell["CellName"]]) { - /* fails silently, sometimes this is undefined, in which case the graph array should be shorter than the cell array, check in reducer */ - positions[i] = [ - glScaleX(nextProps.graphMap[cell["CellName"]][0]), - glScaleY(nextProps.graphMap[cell["CellName"]][1]) - ]; + const graphVec = nextProps.graphVec; + for (var i = 0; i < vertexCount; i++) { + const cell = vertices[i]; + const cellIdx = cell.__cellIndex__; + const x = glScaleX(graphVec[2 * cellIdx]); + const y = glScaleY(graphVec[2 * cellIdx + 1]); + positions[2 * i] = x; + positions[2 * i + 1] = y; - colors[i] = cell.__colorRGB__; - sizes[i] = cell["__selected__"] - ? 4 - : 0.2; /* make this a function of the number of total cells, including regraph */ - } - }); + colors.set(cell.__colorRGB__, 3 * i); - this.state.pointBuffer(positions); - this.state.colorBuffer(colors); - this.state.sizeBuffer(sizes); - this.count = positions.length; + sizes[i] = cell.__selected__ + ? 4 + : 0.2; /* make this a function of the number of total cells, including regraph */ + } + + this.state.pointBuffer({ data: positions, dimension: 2 }); + this.state.colorBuffer({ data: colors, dimension: 3 }); + this.state.sizeBuffer({ data: sizes, dimension: 1 }); + this.count = vertexCount; } } handleBrushSelectAction() { diff --git a/src/components/scatterplot/scatterplot.js b/src/components/scatterplot/scatterplot.js index 3aee9d5e..7582e769 100644 --- a/src/components/scatterplot/scatterplot.js +++ b/src/components/scatterplot/scatterplot.js @@ -15,6 +15,7 @@ import fit from "canvas-fit"; import _camera from "../../util/camera.js"; import _regl from "regl"; import _drawPoints from "./drawPointsRegl"; +import { scaleLinear } from "../../util/scaleLinear"; import { margin, width, height, createDimensions } from "./util"; @@ -39,6 +40,7 @@ import { margin, width, height, createDimensions } from "./util"; colorAccessor: state.controls.colorAccessor, colorScale: state.controls.colorScale, currentCellSelection: state.controls.currentCellSelection, + currentCellSelectionMap: state.controls.currentCellSelectionMap, scatterplotXXaccessor: state.controls.scatterplotXXaccessor, scatterplotYYaccessor: state.controls.scatterplotYYaccessor, opacityForDeselectedCells: state.controls.opacityForDeselectedCells, @@ -107,12 +109,12 @@ class Scatterplot extends React.Component { } componentDidUpdate(prevProps) { if ( - (this.state.xScale && - this.state.yScale && - (this.props.scatterplotXXaccessor && - this.props.scatterplotYYaccessor) && - this.props.scatterplotXXaccessor !== prevProps.scatterplotXXaccessor) || // was CLU now FTH1 etc - this.props.scatterplotYYaccessor !== prevProps.scatterplotYYaccessor + this.state.xScale && + this.state.yScale && + this.props.scatterplotXXaccessor && + this.props.scatterplotYYaccessor && + (this.props.scatterplotXXaccessor !== prevProps.scatterplotXXaccessor || // was CLU now FTH1 etc + this.props.scatterplotYYaccessor !== prevProps.scatterplotYYaccessor) ) { this.drawAxesSVG(this.state.xScale, this.state.yScale); } @@ -130,67 +132,54 @@ class Scatterplot extends React.Component { this.state.xScale && this.state.yScale ) { - const _currentCellSelectionMap = _.keyBy( - this.props.currentCellSelection, - "CellName" - ); /* move me to the reducer */ + const currentCellSelectionMap = this.props.currentCellSelectionMap; - const positions = []; - const colors = []; - const sizes = []; + const data = this.props.expression.data; + const cells = data.cells; + const genes = data.genes; + const cellCount = cells.length; + const positions = new Float32Array(2 * cellCount); + const colors = new Float32Array(3 * cellCount); + const sizes = new Float32Array(cellCount); - const glScaleX = d3 - .scaleLinear() - .domain([0, width]) - .range([-0.95, 0.95]); /* padding */ + // d3.scaleLinear().domain([0, width]).range([-0.95, 0.95]) + const glScaleX = scaleLinear([0, width], [-0.95, 0.95]); - const glScaleY = d3 - .scaleLinear() - .domain([0, height]) - .range([-1, 1]); + // d3.scaleLinear().domain([0, height]).range([-1, 1]) + const glScaleY = scaleLinear([0, height], [-1, 1]); + + const geneXXaccessorIndex = genes.indexOf( + this.props.scatterplotXXaccessor + ); + const geneYYaccessorIndex = genes.indexOf( + this.props.scatterplotYYaccessor + ); /* Construct Vectors */ - _.each(this.props.expression.data.cells, (cell, i) => { - /* - this if is necessary until we are no longer getting expression for all cells, but only for 'world' - ...which will mean refetching when we regraph, or 'go back up to all cells' - */ - if (_currentCellSelectionMap[cell.cellname]) { - /* fails silently, sometimes this is undefined, in which case the graph array should be shorter than the cell array, check in reducer */ - positions.push([ - glScaleX( - this.state.xScale( - cell.e[ - this.props.expression.data.genes.indexOf( - this.props.scatterplotXXaccessor - ) - ] - ) - ) /* scale each point first to the window as we calculate extents separately below, so no need to repeat */, - glScaleY( - this.state.yScale( - cell.e[ - this.props.expression.data.genes.indexOf( - this.props.scatterplotYYaccessor - ) - ] - ) - ) - ]); + for (var i = 0; i < cellCount; i++) { + const cell = cells[i]; + const cellMetadata = currentCellSelectionMap[cell.cellname]; - colors.push(_currentCellSelectionMap[cell.cellname]["__colorRGB__"]); - sizes.push( - _currentCellSelectionMap[cell.cellname]["__selected__"] ? 4 : 0.2 - ); /* make this a function of the number of total cells, including regraph */ - } - }); + positions[2 * i] = glScaleX( + this.state.xScale(cell.e[geneXXaccessorIndex]) + ); /* scale each point first to the window as we calculate extents separately below, so no need to repeat */ + positions[2 * i + 1] = glScaleY( + this.state.yScale(cell.e[geneYYaccessorIndex]) + ); - this.state.pointBuffer(positions); - this.state.colorBuffer(colors); - this.state.sizeBuffer(sizes); - this.count = positions.length; + colors.set(cellMetadata.__colorRGB__, 3 * i); + + sizes[i] = cellMetadata.__selected__ + ? 4 + : 0.2; /* make this a function of the number of total cells, including regraph */ + } + + this.state.pointBuffer({ data: positions, dimension: 2 }); + this.state.colorBuffer({ data: colors, dimension: 3 }); + this.state.sizeBuffer({ data: sizes, dimension: 1 }); + this.count = cellCount; } } maybeSetupScalesAndDrawAxes(nextProps) { diff --git a/src/globals.js b/src/globals.js index bd3bf6ef..962e68a6 100644 --- a/src/globals.js +++ b/src/globals.js @@ -72,21 +72,17 @@ export const graphMargin = { top: 20, right: 10, bottom: 30, left: 40 }; export const graphWidth = 960; export const graphHeight = 960; -export const graphXScale = d3 - .scaleLinear() - .domain([ - 0, - 1 - ]) /* while this is the default for d3, our data is normalized so better to be explicit */ - .range([0 + graphMargin.left, graphWidth - graphMargin.right]); - -export const graphYScale = d3 - .scaleLinear() - .domain([ - 0, - 1 - ]) /* while this is the default for d3, our data is normalized so better to be explicit */ - .range([graphHeight - graphMargin.bottom, 0 + graphMargin.top]); +import { scaleLinear } from "./util/scaleLinear"; +// d3.scaleLinear().domain([0,1]).range([0 + graphMargin.left, graphWidth - graphMargin.right]) +export const graphXScale = scaleLinear( + [0, 1], + [0 + graphMargin.left, graphWidth - graphMargin.right] +); +// d3.scaleLinear().domain([0,1]).range([graphHeight - graphMargin.bottom, 0 + graphMargin.top]) +export const graphYScale = scaleLinear( + [0, 1], + [graphHeight - graphMargin.bottom, 0 + graphMargin.top] +); export const ordinalColors = [ "#0ac115", diff --git a/src/middleware/updateCellColors.js b/src/middleware/updateCellColors.js index 46003647..f6a8ba9f 100644 --- a/src/middleware/updateCellColors.js +++ b/src/middleware/updateCellColors.js @@ -54,11 +54,12 @@ const updateCellSelectionMiddleware = store => { if (action.type === "color by categorical metadata") { colorScale = d3.scaleOrdinal().range(globals.ordinalColors); - _.each(currentSelectionWithUpdatedColors, (cell, i) => { + for (let i = 0; i < currentSelectionWithUpdatedColors.length; i++) { + const cell = currentSelectionWithUpdatedColors[i]; let c = colorScale(cell[action.colorAccessor]); - currentSelectionWithUpdatedColors[i]["__color__"] = c; - currentSelectionWithUpdatedColors[i]["__colorRGB__"] = parseRGB(c); - }); + cell.__color__ = c; + cell.__colorRGB__ = parseRGB(c); + } } if (action.type === "color by continuous metadata") { diff --git a/src/middleware/updateCellSelectionMiddleware.js b/src/middleware/updateCellSelectionMiddleware.js index 6d4e04f1..5ac370f1 100644 --- a/src/middleware/updateCellSelectionMiddleware.js +++ b/src/middleware/updateCellSelectionMiddleware.js @@ -37,7 +37,7 @@ const updateCellSelectionMiddleware = store => { if ( !filterJustChanged || !s.controls.allCellsOnClient - /* graphMap is set at the same time as allCells, so we assume it exists */ + /* graph is set at the same time as allCells, so we assume it exists */ ) { return next( action @@ -46,12 +46,11 @@ const updateCellSelectionMiddleware = store => { /* - make a FRESH copy of all of the cells - - metadata has cellname, and that's all we ever need (is a key to graphMap) + - metadata has cellname and index, and that's all we ever need to reference cell info */ let newSelection = s.controls.currentCellSelection.slice(0); - _.each(newSelection, cell => { - cell["__selected__"] = true; - }); + _.forEach(newSelection, cell => (cell.__selected__ = true)); + /* in plain language... @@ -74,26 +73,23 @@ const updateCellSelectionMiddleware = store => { ? action.brushCoords : s.controls.graphBrushSelection; - _.each(newSelection, (cell, i) => { - if (!s.controls.graphMap[cell["CellName"]]) { - newSelection[i][ - "__selected__" - ] = false; /* make a toggle in future */ - return; - } - - const coords = s.controls.graphMap[cell["CellName"]]; // [0.08005009151334168, 0.6907652173913044] + const graphVec = s.controls.graphVec; + for (let i = 0; i < newSelection.length; i++) { + const cell = newSelection[i]; + const cellId = cell.__cellIndex__; + const x = globals.graphXScale(graphVec[2 * cellId]); + const y = globals.graphYScale(graphVec[2 * cellId + 1]); const pointIsInsideBrushBounds = - globals.graphXScale(coords[0]) >= graphBrushSelection.northwestX && - globals.graphXScale(coords[0]) <= graphBrushSelection.southeastX && - globals.graphYScale(coords[1]) >= graphBrushSelection.northwestY && - globals.graphYScale(coords[1]) <= graphBrushSelection.southeastY; + x >= graphBrushSelection.northwestX && + x <= graphBrushSelection.southeastX && + y >= graphBrushSelection.northwestY && + y <= graphBrushSelection.southeastY; if (!pointIsInsideBrushBounds) { - newSelection[i]["__selected__"] = false; + cell.__selected__ = false; } - }); + } } if ( (action.type === diff --git a/src/reducers/controls.js b/src/reducers/controls.js index 3f514613..47d43ee1 100644 --- a/src/reducers/controls.js +++ b/src/reducers/controls.js @@ -8,8 +8,9 @@ const Controls = ( allGeneNames: null, allCellsOnClient: null /* this comes from cells endpoint, this is world */, currentCellSelection: null /* this comes from user actions, all draw components use this, it is created by middleware */, - graphMap: null, + graphVec: null, categoricalAsBooleansMap: null, + categoricalAsCellsMap: null, colorAccessor: null, colorScale: null, opacityForDeselectedCells: 0.2, @@ -33,12 +34,22 @@ const Controls = ( allGeneNames: action.data.data.genes }); case "request cells success": - const graphMap = {}; - const currentCellSelection = action.data.data.metadata.slice(0); - _.each(action.data.data.graph, g => { - graphMap[g[0]] = [g[1], g[2]]; + // Store the graph in a linear array for fast access. Index into + // the array by "cell index", which is stored as metadata field + // __cellIndex__. + // + // Code below relies on the REST API guarantee that the graph and + // metadata are returned as arrays with the same order and length. + // + const graphVec = new Float32Array(2 * action.data.data.graph.length); + _.each(action.data.data.graph, (g, i) => { + graphVec[2 * i] = g[1]; + graphVec[2 * i + 1] = g[2]; }); + const currentCellSelection = action.data.data.metadata.slice(0); + const currentCellSelectionMap = _.keyBy(currentCellSelection, "CellName"); + /* construct a copy of the ranges object that only has categorical replace all counts with bool flags @@ -66,11 +77,12 @@ const Controls = ( } }); - _.each(currentCellSelection, cell => { - cell["__selected__"] = true; - cell["__color__"] = + _.each(currentCellSelection, (cell, idx) => { + cell.__cellIndex__ = idx; + cell.__selected__ = true; + cell.__color__ = "rgba(0,0,0,1)"; /* initial color for all cells in all charts */ - cell["__colorRGB__"] = parseRGB(cell["__color__"]); + cell.__colorRGB__ = parseRGB(cell.__color__); // Add each cell to its categorical metadata set. _.forEach(cell, (_value, key) => { @@ -87,7 +99,8 @@ const Controls = ( return Object.assign({}, state, { allCellsOnClient: action.data.data, currentCellSelection, - graphMap, + currentCellSelectionMap, + graphVec, categoricalAsBooleansMap, categoricalAsCellsMap, continuousUserDefinedRanges, diff --git a/src/util/scaleLinear.js b/src/util/scaleLinear.js new file mode 100644 index 00000000..daff5ffe --- /dev/null +++ b/src/util/scaleLinear.js @@ -0,0 +1,18 @@ +// jshint esversion: 6 + +// Substitute for a d3 linear scale - less flexible, more performant. +// Returns a function which will scale a value. +// +// Example will scale [0,1] to [-1,1] +// var myScale = scaleLinear([0, 1], [-1, 1]); +// myScale(0) === -1 +// this is is equivalent to d3.scaleLinear().domain([0,1]).range([-1,1]) + +export const scaleLinear = (domain, range) => { + const offsetD = domain[0]; + const scale = (range[1] - range[0]) / (domain[1] - domain[0]); + const offsetR = range[0]; + return function(v) { + return (v - offsetD) * scale + offsetR; + }; +}; From 06a5eedd0e456ffbb9fc20170d4b00991656a83c Mon Sep 17 00:00:00 2001 From: bkmartinjr Date: Wed, 9 May 2018 21:10:59 -0700 Subject: [PATCH 2/2] performance - remove scaling from inner render loop --- src/globals.js | 8 ++++++ .../updateCellSelectionMiddleware.js | 25 ++++++++++++++----- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/globals.js b/src/globals.js index 962e68a6..bd6986ba 100644 --- a/src/globals.js +++ b/src/globals.js @@ -78,11 +78,19 @@ export const graphXScale = scaleLinear( [0, 1], [0 + graphMargin.left, graphWidth - graphMargin.right] ); +graphXScale.invert = scaleLinear( + [0 + graphMargin.left, graphWidth - graphMargin.right], + [0, 1] +); // d3.scaleLinear().domain([0,1]).range([graphHeight - graphMargin.bottom, 0 + graphMargin.top]) export const graphYScale = scaleLinear( [0, 1], [graphHeight - graphMargin.bottom, 0 + graphMargin.top] ); +graphYScale.invert = scaleLinear( + [graphHeight - graphMargin.bottom, 0 + graphMargin.top], + [0, 1] +); export const ordinalColors = [ "#0ac115", diff --git a/src/middleware/updateCellSelectionMiddleware.js b/src/middleware/updateCellSelectionMiddleware.js index 5ac370f1..7d507dd4 100644 --- a/src/middleware/updateCellSelectionMiddleware.js +++ b/src/middleware/updateCellSelectionMiddleware.js @@ -73,18 +73,31 @@ const updateCellSelectionMiddleware = store => { ? action.brushCoords : s.controls.graphBrushSelection; + const northwestX = globals.graphXScale.invert( + graphBrushSelection.northwestX + ); + const southeastX = globals.graphXScale.invert( + graphBrushSelection.southeastX + ); + const northwestY = globals.graphYScale.invert( + graphBrushSelection.northwestY + ); + const southeastY = globals.graphYScale.invert( + graphBrushSelection.southeastY + ); + const graphVec = s.controls.graphVec; for (let i = 0; i < newSelection.length; i++) { const cell = newSelection[i]; const cellId = cell.__cellIndex__; - const x = globals.graphXScale(graphVec[2 * cellId]); - const y = globals.graphYScale(graphVec[2 * cellId + 1]); + const x = graphVec[2 * cellId]; + const y = graphVec[2 * cellId + 1]; const pointIsInsideBrushBounds = - x >= graphBrushSelection.northwestX && - x <= graphBrushSelection.southeastX && - y >= graphBrushSelection.northwestY && - y <= graphBrushSelection.southeastY; + x >= northwestX && + x <= southeastX && + y <= northwestY && + y >= southeastY; if (!pointIsInsideBrushBounds) { cell.__selected__ = false;