Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-10830: Optimize the performance of multi-trace chart updates #397

Merged
merged 9 commits into from
Jun 29, 2017
Merged

DM-10830: Optimize the performance of multi-trace chart updates #397

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
c8743ad
DM-10830: Optimize the performance of multi-trace chart updates
loitly Jun 23, 2017
d76da65
fixes highlighting selected, bug due to shared data
tgoldina Jun 27, 2017
eadf0f8
DM-10830: fixes made per review
loitly Jun 27, 2017
f1e8e66
DM-10830: additinal fixes
loitly Jun 27, 2017
f266326
- add back the non-supported symbols since we're not using GL.
loitly Jun 27, 2017
ab8f390
- fix wrong use of boolean function.
loitly Jun 27, 2017
464634f
- fixed FilterInfo.js fail when comparing sparsed array(rows).
loitly Jun 29, 2017
313a4f5
- remove old plotly.js file
loitly Jun 29, 2017
99d2125
Merge branch 'dev' into DM-10830_optimize_plotly_chart
loitly Jun 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/firefly/html/demo/plotly-concepts.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
<link rel=”apple-touch-startup-image” href=”images/fftools-ipad_splash_768x1004.png”>
<title>Plotly Concept</title>

<script type="text/javascript" src="../plotly-1.27.1.min.js"></script>
<script type="text/javascript" src="../plotly-1.28.2.min.js"></script>
</head>

<body style="margin: 0; background-color: rgb(200,200,200)">
Expand Down
78 changes: 78 additions & 0 deletions src/firefly/html/plotly-1.28.2.min.js
View file
Open in desktop

Large diffs are not rendered by default.

23 changes: 18 additions & 5 deletions src/firefly/js/charts/ChartUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* Utilities related to charts
* Created by tatianag on 3/17/16.
*/
import {get, uniqueId, isUndefined, omitBy, zip, isEmpty, range, set, isObject, isPlainObject, pick, cloneDeep, merge} from 'lodash';
import {get, uniqueId, isUndefined, omitBy, zip, isEmpty, range, set, isObject, pick, cloneDeep, merge} from 'lodash';
import shallowequal from 'shallowequal';

import {flux} from '../Firefly.js';
Expand Down Expand Up @@ -278,7 +278,7 @@ export function getOptionsUI(chartId) {
const {data, fireflyData, activeTrace=0} = getChartData(chartId);
const type = get(data, [activeTrace, 'type'], 'scatter');
const dataType = get(fireflyData, [activeTrace, 'dataType'], '');
if (type === 'scatter') {
if (type.includes('scatter')) {
return ScatterOptions;
} else if (type === 'histogram') {
return HistogramOptions;
Expand All @@ -292,7 +292,7 @@ export function getOptionsUI(chartId) {
export function getToolbarUI(chartId, activeTrace=0) {
const {data, fireflyData} = getChartData(chartId);
const type = get(data, [activeTrace, 'type'], '');
if (type === 'scatter') {
if (type.includes('scatter')) {
return ScatterToolbar;
} else {
return BasicToolbar;
Expand Down Expand Up @@ -327,7 +327,7 @@ export function newTraceFrom(data, selIndexes, newTraceProps) {
});
}
deepReplace(sdata);
const flatprops = flattenObject(newTraceProps, isPlainObject);
const flatprops = flattenObject(newTraceProps);
Object.entries(flatprops).forEach(([k,v]) => set(sdata, k, v));
return sdata;
}
Expand Down Expand Up @@ -474,7 +474,7 @@ function makeTableSources(chartId, data=[], fireflyData=[]) {
const currentData = (data.length < fireflyData.length) ? fireflyData : data;

return currentData.map((d, traceNum) => {
const ds = data[traceNum] ? convertToDS(flattenObject(data[traceNum], isPlainObject)) : {}; //avoid flattening arrays
const ds = data[traceNum] ? convertToDS(flattenObject(data[traceNum])) : {}; //avoid flattening arrays
if (!ds.tbl_id) {
// table id can be a part of fireflyData
const tbl_id = get(fireflyData, `${traceNum}.tbl_id`);
Expand Down Expand Up @@ -548,6 +548,19 @@ export function applyDefaults(chartData={}) {
chartData.layout = merge(defaultLayout, chartData.layout);
}

const TRACE_COLORS = ['lightblue', 'green', 'purple', 'brown', 'yellow', 'red'];
export function getNewTraceDefaults(type='', traceNum=0) {
if (type.includes(SCATTER)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, when the type is not SCATTER this function returns undefined, which is causing the type error, which I mentioned before (when we are trying to use the result as an array).

return {
[`data.${traceNum}.type`] : type, //make sure trace type is set
[`data.${traceNum}.marker.color`]: TRACE_COLORS[traceNum] || undefined,
[`data.${traceNum}.marker.line`]: 'none',
[`data.${traceNum}.showlegend`]: true,
['layout.xaxis.range'] : undefined, //clear out fixed range
['layout.yaxis.range'] : undefined, //clear out fixed range
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that the defaults behave differently for scatter and scattergl.
For example yaxis: {ticklen: 5 } shows the tick, but it overlaps with the tick label for scattergl. It looks like the middle of the tick label rather then the edge is positioned next to the tick. Looks like a Plotly bug. The workaround is not to show the axis line or ticks, and show the grid instead.

Also, in scattergl, the spikes (vertical and horizontal droplines meeting at the point) are shown by default, while they are not shown by default in scatter plot. For crowded charts these lines are convenient, but since the spikes do not extend all the way, the direction is wrong when axes are on the opposite sides. None of the spike attributes listed in the documentation take effect for scattergl at the moment. mirror axis attribute does not work either. So the workaround would be to remove the options for opposite axis location. (As well as for reverse.)




Expand Down
14 changes: 13 additions & 1 deletion src/firefly/js/charts/ChartsCntlr.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ function chartHighlight(action) {
const {activeTrace=activeDataTrace} = action.payload; // activeTrace can be selected or highlighted trace of the data trace
const ttype = get(data, [activeTrace, 'type'], 'scatter');

if (!isEmpty(tablesources) && ttype === 'scatter') {
if (!isEmpty(tablesources) && ttype.includes('scatter')) {
// activeTrace is different from activeDataTrace if a selected point highlighted, for example
const {tbl_id} = tablesources[activeDataTrace] || {};
if (!tbl_id) return;
Expand Down Expand Up @@ -613,6 +613,11 @@ export function reducer(state={ui:{}, data:{}}, action={}) {
// TablesCntlr.TABLE_REMOVE, TablesCntlr.TABLE_SELECT];


function changeToScatterGL(chartData) {
get(chartData, 'data', []).forEach((d) => d.type === 'scatter' && (d.type = 'scattergl')); // use scattergl instead of scatter
['selected', 'highlighted'].map((k) => get(chartData, k, {})).forEach((d) => d.type === 'scatter' && (d.type = 'scattergl'));
}

/**
* @param state - ui part of chart state
* @param action - action
Expand All @@ -629,6 +634,9 @@ function reduceData(state={}, action={}) {
if (chartType==='plot.ly') {
rest['_original'] = cloneDeep(action.payload);
applyDefaults(rest);
if (!sessionStorage.getItem('noScatterGL')) {
changeToScatterGL(rest);
}
}
state = updateSet(state, chartId,
omitBy({
Expand All @@ -644,6 +652,10 @@ function reduceData(state={}, action={}) {
const {chartId, changes} = action.payload;
var chartData = getChartData(chartId) || {};
chartData = updateObject(chartData, changes);

if (!sessionStorage.getItem('noScatterGL')) {
changeToScatterGL(chartData);
}
return updateSet(state, chartId, chartData);
}
case (CHART_REMOVE) :
Expand Down
2 changes: 1 addition & 1 deletion src/firefly/js/charts/PlotlyConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export default Plotly;



const PLOTLY_SCRIPT= 'plotly-1.27.1.min.js';
const PLOTLY_SCRIPT= 'plotly-1.28.2.min.js';
const LOAD_ERR_MSG= 'Load Failed: could not load Plotly';

function initPlotLyRetriever(loadNow) {
Expand Down
9 changes: 7 additions & 2 deletions src/firefly/js/charts/ui/PlotlyChartArea.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,16 @@ export class PlotlyChartArea extends PureComponent {
render() {
const {widthPx, heightPx} = this.props;
const {data=[], highlighted, selected, layout={}, activeTrace=0} = this.state;
const doingResize= (layout && (layout.width!==widthPx || layout.height!==heightPx));
let doingResize = false;
if (widthPx !== this.widthPx || heightPx !== this.heightPx) {
this.widthPx = widthPx;
this.heightPx = heightPx;
doingResize = true;
}
const showlegend = data.length > 1;
let pdata = data;
// TODO: change highlight or selected without forcing new plot
if (!data[activeTrace] || get(data[activeTrace], 'type') === 'scatter') {
if (!data[activeTrace] || get(data[activeTrace], 'type', '').includes('scatter')) {
// highlight makes sence only for scatter at the moment
pdata = selected ? pdata.concat([selected]) : pdata;
pdata = highlighted ? pdata.concat([highlighted]) : pdata;
Expand Down
5 changes: 3 additions & 2 deletions src/firefly/js/charts/ui/PlotlyToolbar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,9 @@ function SelectBtn({style={}, chartId, dragmode}) {
function ResetZoomBtn({style={}, chartId}) {
const {_original} = getChartData(chartId) || {};
const doClick = () => {
const changes = ['xaxis','yaxis','zaxis'].reduce((pv, axis) => {
pv[`layout.${axis}.autorange`] = get(_original, `layout.${axis}.autorange`);
// TODO: this only handles chart with 2 axes
const changes = ['xaxis','yaxis'].reduce((pv, axis) => {
pv[`layout.${axis}.autorange`] = get(_original, `layout.${axis}.autorange`, true);
pv[`layout.${axis}.range`] = get(_original, `layout.${axis}.range`);
return pv;
}, {});
Expand Down
89 changes: 81 additions & 8 deletions src/firefly/js/charts/ui/PlotlyWrapper.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@

import React, {Component} from 'react';
import PropTypes from 'prop-types';
import {get,debounce} from 'lodash';
import {get, debounce, isEmpty, set, omit} from 'lodash';
import {getPlotLy} from '../PlotlyConfig.js';
import {logError} from '../../util/WebUtil.js';
import {getChartData} from '../ChartsCntlr.js';
import {logError, deltas, flattenObject} from '../../util/WebUtil.js';
import BrowserInfo from '../../util/BrowserInfo.js';
import Enum from 'enum';

Expand Down Expand Up @@ -166,24 +167,58 @@ export class PlotlyWrapper extends Component {
return true;
}

optimize(graphDiv, renderType, {chartId, data, layout, dataUpdate, layoutUpdate, dataUpdateTraces, ...rest}) {
const {lastInputTime} = layout;
if (lastInputTime && renderType === RenderType.NEW_PLOT && graphDiv.data) {
// omitting 'firefly' from data[*] for now
data = data.map((d) => omit(d, 'firefly'));
Copy link
Contributor

@tgoldina tgoldina Jun 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the mismatch in clicked and highlighted point when highlighting a selected point is caused by not passing firefly rowIdx array in plotly data. (For selected points I get row idx from plotly div, because we do not save them in data.) I think, we should omit firefly object for the purposes of calculating the difference, but it should not be dropped completely. We should still pass it with the partial or full update. Alternatively, we should store selected and highlighted traces in our store.

layout = omit(layout, 'lastInputTime');

const dataDelta = deltas(data, graphDiv.data || {});
const layoutDelta = flattenObject(deltas(layout, graphDiv.layout || {}, false));

const hasLayout = !isEmpty(layoutDelta);
const hasData = !isEmpty(dataDelta);
if(hasData) {
dataUpdate = Object.values(dataDelta).map((d) => flattenObject(d));
dataUpdateTraces = Object.keys(dataDelta).map((k) => parseInt(k));
renderType = RenderType.RESTYLE;
}
if (hasLayout) {
layoutUpdate = layoutDelta;
renderType = RenderType.RELAYOUT;
}
if (hasData && hasLayout) {
renderType = RenderType.RESTYLE_AND_RELAYOUT;
}
}

return {renderType, chartId, data, layout, dataUpdate, layoutUpdate, dataUpdateTraces, ...rest};
}

draw() {
const renderType = this.renderType;
let renderType = this.renderType;
if (renderType===RenderType.PAUSE_DRAWING) return;

const {data,layout, config= defaultConfig, newPlotCB, dataUpdate, layoutUpdate, dataUpdateTraces}= this.props;

getPlotLy().then( (Plotly) => {

const optimized = sessionStorage.getItem('chartRedraw') ? Object.assign({renderType}, this.props) :
(this.optimize(this.div || {}, renderType, this.props));

const {chartId, data,layout, config= defaultConfig, newPlotCB, dataUpdate, layoutUpdate, dataUpdateTraces} = optimized;
renderType = optimized.renderType;

if (this.div) { // make sure the div is still there
const now = Date.now();
switch (renderType) {
case RenderType.RESTYLE:
Plotly.restyle(this.div, dataUpdate, dataUpdateTraces);
this.restyle(this.div, Plotly, dataUpdate, dataUpdateTraces);
break;
case RenderType.RELAYOUT:
Plotly.relayout(this.div, layoutUpdate);
break;
case RenderType.RESTYLE_AND_RELAYOUT:
Plotly.restyle(this.div, dataUpdate, dataUpdateTraces);
this.restyle(this.div, Plotly, dataUpdate, dataUpdateTraces);
Plotly.relayout(this.div, layoutUpdate);
break;
case RenderType.RESIZE:
Expand All @@ -202,6 +237,7 @@ export class PlotlyWrapper extends Component {
chart.on('plotly_relayout', () => this.showMask(false));
chart.on('plotly_restyle', () => this.showMask(false));
chart.on('plotly_redraw', () => this.showMask(false));
chart.on('plotly_relayout', (changes) => this.syncLayout(chartId, changes));
}
else {
this.showMask(false);
Expand All @@ -210,11 +246,48 @@ export class PlotlyWrapper extends Component {

break;
}
this.syncLayout(chartId, {lastInputTime: Date.now()});
console.log(`redraw elapsed: ${Date.now() - now}`);
}
} ).catch( (e) => {
console.log('Plotly not loaded',e);
});
}

/**
* This function sync the div.layout with chart's layout.
* Will use direct object update instead of dispatch chart update to avoid
* unneeded render/comparison.
* @param chartId
* @param changes
*/
syncLayout(chartId, changes) {
const {layout} = getChartData(chartId) || {};
if (layout) {
Object.entries(changes).forEach( ([k, v]) => {
if (k === 'xaxis' && Array.isArray(v)) {
set(layout, 'xaxis.range', v);
set(layout, 'xaxis.autorange', false);
} else if (k === 'yaxis' && Array.isArray(v)) {
set(layout, 'yaxis.range', v);
set(layout, 'yaxis.autorange', false);
} else {
set(layout, k, v);
}
});
}
}

restyle(div, Plotly, dataUpdate, dataUpdateTraces) {
if (Array.isArray(dataUpdate)) {
dataUpdate.forEach((v,idx) => this.restyle(div, Plotly, v, dataUpdateTraces[idx]));
} else {
if (dataUpdateTraces >= get(div, 'data.length', 0)) {
Plotly.addTraces(div, dataUpdate, dataUpdateTraces);
} else {
Plotly.restyle(div, dataUpdate, dataUpdateTraces);
}
}
}

refUpdate(ref) {
Expand Down Expand Up @@ -279,7 +352,7 @@ PlotlyWrapper.defaultProps = {
maskOnLayout : true,
maskOnRestyle : false,
maskOnResize : true,
maskOnNewPlot : true,
maskOnNewPlot : false,

autoSizePlot : false,
autoDetectResizing : false,
Expand Down
8 changes: 8 additions & 0 deletions src/firefly/js/charts/ui/options/NewTracePanel.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';

import {getChartData} from '../../ChartsCntlr.js';
import {getNewTraceDefaults} from '../../ChartUtil.js';
import {FieldGroup} from '../../../ui/FieldGroup.jsx';
import {ValidationField} from '../../../ui/ValidationField.jsx';
import {ListBoxInputField} from '../../../ui/ListBoxInputField.jsx';
Expand All @@ -20,6 +21,7 @@ const fieldProps = {labelWidth: 62, size: 15};
function getSubmitChangesFunc(traceType) {
switch(traceType) {
case 'scatter':
case 'scattergl':
return submitChangesScatter;
case 'fireflyHistogram':
return submitChangesFFHistogram;
Expand All @@ -32,6 +34,7 @@ function getOptionsComponent({traceType, chartId, activeTrace, groupKey}) {
const {data, layout} = getChartData(chartId);
switch(traceType) {
case 'scatter':
case 'scattergl':
return (<ScatterOptions {...{chartId, activeTrace, groupKey}}/>);
case 'fireflyHistogram':
return (<FireflyHistogramOptions {...{chartId, activeTrace, groupKey}}/>);
Expand Down Expand Up @@ -70,6 +73,11 @@ export class NewTracePanel extends SimpleComponent {

fields = Object.assign({activeTrace}, fields); // make the newly added trace active
fields[`data.${activeTrace}.type`] = type; //make sure trace type is set

// apply defaults settings
Object.entries(getNewTraceDefaults(type, activeTrace))
.forEach(([k,v]) => !fields[k] && (fields[k] = v));

// need to hide before the changes are submitted to avoid React Internal error too much recursion (mounting/unmouting fields)
dispatchHideDialog('ScatterNewTracePanel');
submitChangesFunc({chartId, activeTrace, fields, tbl_id});
Expand Down
4 changes: 4 additions & 0 deletions src/firefly/js/charts/ui/options/ScatterOptions.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,12 @@ export class ScatterOptions extends SimpleComponent {
<FieldGroup className='FieldGroup__vertical' keepState={false} groupKey={groupKey} reducerFunc={fieldReducer({data, layout, activeTrace, tablesources})}>
<ListBoxInputField fieldKey={`data.${activeTrace}.mode`} options={[{value:'markers'}, {value:'lines'}, {value:'lines+markers'}]}/>
<ListBoxInputField fieldKey={`data.${activeTrace}.marker.symbol`}
options={[{value:'circle'}, {value:'square'}, {value:'diamond'},
{value:'cross'}, {value:'x'}, {value:'triangle-up'}, {value:'hexagon'}, {value:'star'}]}/>
{/* TODO: scattergl does not support 'open' symbols as of v1..28.2. we'll add them back at a later time when they do.
options={[{value:'circle'}, {value:'circle-open'}, {value:'square'}, {value:'square-open'}, {value:'diamond'}, {value:'diamond-open'},
{value:'cross'}, {value:'x'}, {value:'triangle-up'}, {value:'hexagon'}, {value:'star'}]}/>
*/}
{tablesource && <TableSourcesOptions {...{tablesource, activeTrace, groupKey}}/>}
<br/>
<BasicOptionFields {...{layout, data, activeTrace}}/>
Expand Down
Loading