From f826479b0d41aa1ec8ae027275eebc61c1a9584b Mon Sep 17 00:00:00 2001 From: Matthew Runyon Date: Fri, 10 May 2024 18:40:17 -0500 Subject: [PATCH] Fix fetching over 1 million points if it can be downsampled --- .../js/src/PlotlyExpressChartModel.test.ts | 77 ++++++++++++++++--- .../src/js/src/PlotlyExpressChartModel.ts | 54 +++++++------ 2 files changed, 94 insertions(+), 37 deletions(-) diff --git a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts index 57ef108b5..5b3df50ff 100644 --- a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts +++ b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.test.ts @@ -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'; @@ -20,11 +21,28 @@ const SMALL_TABLE = TestUtils.createMockProxy({ const LARGE_TABLE = TestUtils.createMockProxy({ columns: [{ name: 'x' }, { name: 'y' }] as DhType.Column[], - size: 50000, + size: 50_000, + subscribe: () => TestUtils.createMockProxy(), +}); + +const REALLY_LARGE_TABLE = TestUtils.createMockProxy({ + 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 = {}; + 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: { @@ -48,10 +66,7 @@ function createMockWidget(tables: DhType.Table[], plotType = 'scatter') { })), layout: { title: 'layout', - xaxis: {}, - yaxis: {}, - xaxis2: {}, - yaxis2: {}, + ...layoutAxes, }, }, }, @@ -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); }); @@ -156,14 +171,18 @@ 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, @@ -171,10 +190,10 @@ describe('PlotlyExpressChartModel', () => { ); 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 () => { @@ -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); }); diff --git a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts index 2b88d0120..0695eef4c 100644 --- a/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts +++ b/plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts @@ -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, @@ -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) { @@ -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