Skip to content

Commit

Permalink
Fix fetching over 1 million points if it can be downsampled
Browse files Browse the repository at this point in the history
  • Loading branch information
@mattrunyon
mattrunyon committed May 10, 2024
1 parent 750a56e commit f826479
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 37 deletions.
77 changes: 65 additions & 12 deletions plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { Layout } from 'plotly.js';
import { dh as DhType } from '@deephaven/jsapi-types';
import { TestUtils } from '@deephaven/utils';
import { PlotlyExpressChartModel } from './PlotlyExpressChartModel';
Expand All @@ -20,11 +21,28 @@ const SMALL_TABLE = TestUtils.createMockProxy<DhType.Table>({

const LARGE_TABLE = TestUtils.createMockProxy<DhType.Table>({
columns: [{ name: 'x' }, { name: 'y' }] as DhType.Column[],
size: 50000,
size: 50_000,
subscribe: () => TestUtils.createMockProxy(),
});

const REALLY_LARGE_TABLE = TestUtils.createMockProxy<DhType.Table>({
columns: [{ name: 'x' }, { name: 'y' }] as DhType.Column[],
size: 5_000_000,
subscribe: () => TestUtils.createMockProxy(),
});

function createMockWidget(tables: DhType.Table[], plotType = 'scatter') {
const layoutAxes: Partial<Layout> = {};
tables.forEach((_, i) => {
if (i === 0) {
layoutAxes.xaxis = {};
layoutAxes.yaxis = {};
} else {
layoutAxes[`xaxis${i + 1}` as 'xaxis'] = {};
layoutAxes[`yaxis${i + 1}` as 'yaxis'] = {};
}
});

const widgetData = {
type: 'test',
figure: {
Expand All @@ -48,10 +66,7 @@ function createMockWidget(tables: DhType.Table[], plotType = 'scatter') {
})),
layout: {
title: 'layout',
xaxis: {},
yaxis: {},
xaxis2: {},
yaxis2: {},
...layoutAxes,
},
},
},
Expand Down Expand Up @@ -141,8 +156,8 @@ describe('PlotlyExpressChartModel', () => {
);

await chartModel.subscribe(jest.fn());
expect(mockDownsample).toHaveBeenCalledTimes(0);
await new Promise(process.nextTick); // Subscribe and addTable are async
expect(mockDownsample).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0);
});
Expand All @@ -156,25 +171,29 @@ describe('PlotlyExpressChartModel', () => {
);

await chartModel.subscribe(jest.fn());
expect(mockDownsample).toHaveBeenCalledTimes(1);
await new Promise(process.nextTick); // Subscribe and addTable are async
expect(mockDownsample).toHaveBeenCalledTimes(1);
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(1);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(1);
});

it('should downsample only the required tables', async () => {
const mockWidget = createMockWidget([SMALL_TABLE, LARGE_TABLE]);
const mockWidget = createMockWidget([
SMALL_TABLE,
LARGE_TABLE,
REALLY_LARGE_TABLE,
]);
const chartModel = new PlotlyExpressChartModel(
mockDh,
mockWidget,
jest.fn()
);

await chartModel.subscribe(jest.fn());
expect(mockDownsample).toHaveBeenCalledTimes(1);
await new Promise(process.nextTick); // Subscribe and addTable are async
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(1);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(1);
expect(mockDownsample).toHaveBeenCalledTimes(2);
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(2);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(2);
});

it('should fail to downsample for non-line plots', async () => {
Expand All @@ -186,9 +205,43 @@ describe('PlotlyExpressChartModel', () => {
);

await chartModel.subscribe(jest.fn());
await new Promise(process.nextTick); // Subscribe and addTable are async
expect(mockDownsample).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFail).toHaveBeenCalledTimes(1);
});

it('should fetch non-line plots under the max threshold with downsampling disabled', async () => {
const mockWidget = createMockWidget([LARGE_TABLE], 'scatterpolar');
const chartModel = new PlotlyExpressChartModel(
mockDh,
mockWidget,
jest.fn()
);

chartModel.isDownsamplingDisabled = true;
await chartModel.subscribe(jest.fn());
await new Promise(process.nextTick); // Subscribe and addTable are async
expect(mockDownsample).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFail).toHaveBeenCalledTimes(0);
});

it('should not fetch non-line plots over the max threshold with downsampling disabled', async () => {
const mockWidget = createMockWidget([REALLY_LARGE_TABLE], 'scatterpolar');
const chartModel = new PlotlyExpressChartModel(
mockDh,
mockWidget,
jest.fn()
);

chartModel.isDownsamplingDisabled = true;
await chartModel.subscribe(jest.fn());
await new Promise(process.nextTick); // Subscribe and addTable are async
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(1);
expect(mockDownsample).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleStart).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFinish).toHaveBeenCalledTimes(0);
expect(chartModel.fireDownsampleFail).toHaveBeenCalledTimes(1);
});
Expand Down
54 changes: 29 additions & 25 deletions plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,17 @@ import {
removeColorsFromData,
} from './PlotlyExpressChartUtils';

const AUTO_DOWNSAMPLE_SIZE = 30000;

const MAX_FETCH_SIZE = 1_000_000;

const log = Log.module('@deephaven/js-plugin-plotly-express.ChartModel');

export class PlotlyExpressChartModel extends ChartModel {
static AUTO_DOWNSAMPLE_SIZE = 30000;

static MAX_FETCH_SIZE = 1_000_000;

static canFetch(table: DhType.Table): boolean {
return table.size <= PlotlyExpressChartModel.MAX_FETCH_SIZE;
}

constructor(
dh: typeof DhType,
widget: DhType.Widget,
Expand Down Expand Up @@ -284,25 +288,33 @@ export class PlotlyExpressChartModel extends ChartModel {

let tableToAdd = table;

if (!this.canFetch(table)) {
log.debug(`Table ${id} too big to fetch ${table.size} items`);
this.fireDownsampleFail(
`Too many items to plot: ${Number(table.size).toLocaleString()} items.`
);
return;
}

if (this.needsDownsample(table)) {
this.fireDownsampleStart(null);
const downsampleInfo = this.getDownsampleInfo(id, table);

if (typeof downsampleInfo === 'string') {
const downsampleInfo = this.getDownsampleInfo(id, table);
const needsDownsample =
table.size > PlotlyExpressChartModel.AUTO_DOWNSAMPLE_SIZE;
const canDownsample = typeof downsampleInfo !== 'string';
const canFetch = PlotlyExpressChartModel.canFetch(table);
const shouldDownsample = needsDownsample && !this.isDownsamplingDisabled;

if (!canDownsample) {
if (!canFetch) {
log.debug(`Table ${id} too big to fetch ${table.size} items`);
this.fireDownsampleFail(
`Too many items to plot: ${Number(
table.size
).toLocaleString()} items.`
);
return;
}
if (shouldDownsample) {
this.fireDownsampleFail(downsampleInfo);
return;
}
}

if (canDownsample && needsDownsample) {
this.downsampleMap.set(id, downsampleInfo);
try {
this.fireDownsampleStart(null);
tableToAdd = await downsample(this.dh, downsampleInfo);
this.fireDownsampleFinish(null);
} catch (e) {
Expand Down Expand Up @@ -364,14 +376,6 @@ export class PlotlyExpressChartModel extends ChartModel {
}
}

needsDownsample(table: DhType.Table): boolean {
return !this.isDownsamplingDisabled && table.size > AUTO_DOWNSAMPLE_SIZE;
}

canFetch(table: DhType.Table): boolean {
return table.size <= MAX_FETCH_SIZE;
}

/**
* Gets info on how to downsample a table for plotting.
* @param tableId The tableId to get downsample info for
Expand Down

0 comments on commit f826479

Please sign in to comment.