From 73e0b658edffc7ef89b3b786f3fe30c0e64c96f9 Mon Sep 17 00:00:00 2001 From: Mike Bender Date: Tue, 19 Dec 2023 10:11:55 -0500 Subject: [PATCH] fix: `figure_title` and `chart_title` were not mapped up correctly (#1676) - Previously for some reason we were mapping the first subplot chart title to the figure title, which was incorrect. Instead map the Figure title attribute to the figure, and then chart title to the individual subplots - Annoyingly plotly doesn't have a chart title property, so we need to map these titles to `annotations` to get it to work correctly - Fixes #1674 , Fixes #1675 - Tested with the following snippet: ``` from deephaven.plot.figure import Figure from deephaven import read_csv insurance = read_csv("https://media.githubusercontent.com/media/deephaven/examples/main/Insurance/csv/insurance.csv") insurance_by_region = insurance.view(formulas=["region", "expenses"]).avg_by(["region"]) insurance_by_sex = insurance.view(formulas=["sex", "expenses"]).avg_by(["sex"]) insurance_by_children = insurance.view(formulas=["children", "expenses"]).avg_by(["children"]).sort(order_by=["children"]) insurance_by_smoker = insurance.view(["smoker", "expenses"]).avg_by(["smoker"]) insurance_cat_plots = Figure(rows=2, cols=2).\ new_chart(row=0, col=0).\ plot_cat(series_name="Region", t=insurance_by_region, category="region", y="expenses").\ chart_title(title="Average charge ($) by region").\ new_chart(row=0, col=1).\ plot_cat(series_name="Sex", t=insurance_by_sex, category="sex", y="expenses").\ chart_title(title="Average charge ($) by sex").\ new_chart(row=1, col=0).\ plot_cat(series_name="Children", t=insurance_by_children, category="children", y="expenses").\ chart_title(title="Average charge ($) per number of children").\ new_chart(row=1, col=1).\ plot_cat(series_name="Smoker", t=insurance_by_smoker, category="smoker", y="expenses").\ chart_title(title="Average charge ($) smokers vs nonsmokers").\ chart_legend(visible=False).\ figure_title("Figure Title").\ show() ``` ![image](https://github.com/deephaven/web-client-ui/assets/4505624/f9a6f7fc-3110-492a-8d57-1d49616ce387) --- packages/chart/src/ChartTestUtils.ts | 4 +- packages/chart/src/ChartUtils.ts | 10 +++ packages/chart/src/FigureChartModel.test.ts | 9 ++- packages/chart/src/FigureChartModel.ts | 75 +++++++++++++++++---- 4 files changed, 84 insertions(+), 14 deletions(-) diff --git a/packages/chart/src/ChartTestUtils.ts b/packages/chart/src/ChartTestUtils.ts index 406f15a199..dc7591c942 100644 --- a/packages/chart/src/ChartTestUtils.ts +++ b/packages/chart/src/ChartTestUtils.ts @@ -11,6 +11,8 @@ import type { } from '@deephaven/jsapi-types'; class ChartTestUtils { + static DEFAULT_FIGURE_TITLE = 'Figure Title'; + static DEFAULT_CHART_TITLE = 'Chart Title'; static DEFAULT_X_TITLE = 'X Axis'; @@ -127,7 +129,7 @@ class ChartTestUtils { } makeFigure({ - title = 'Figure', + title = ChartTestUtils.DEFAULT_FIGURE_TITLE, charts = [this.makeChart()], rows = 1, cols = 1, diff --git a/packages/chart/src/ChartUtils.ts b/packages/chart/src/ChartUtils.ts index eb5973ad39..25bb1c0df0 100644 --- a/packages/chart/src/ChartUtils.ts +++ b/packages/chart/src/ChartUtils.ts @@ -421,6 +421,16 @@ class ChartUtils { ); } + /** + * Get the axis type map for the figure provided + * @param figure Figure to get the type map for + * @returns Axis type map for the figure provided + */ + static getAxisTypeMap(figure: Figure): AxisTypeMap { + const axes = ChartUtils.getAllAxes(figure); + return ChartUtils.groupArray(axes, 'type'); + } + /** * Retrieve the chart that contains the passed in series from the figure * @param figure The figure to retrieve the chart from diff --git a/packages/chart/src/FigureChartModel.test.ts b/packages/chart/src/FigureChartModel.test.ts index 2993f981f9..de7f4a5275 100644 --- a/packages/chart/src/FigureChartModel.test.ts +++ b/packages/chart/src/FigureChartModel.test.ts @@ -26,7 +26,7 @@ it('populates the layout properly', () => { expect(model.getLayout()).toEqual( expect.objectContaining({ title: expect.objectContaining({ - text: ChartTestUtils.DEFAULT_CHART_TITLE, + text: ChartTestUtils.DEFAULT_FIGURE_TITLE, }), xaxis: expect.objectContaining({ title: expect.objectContaining({ @@ -38,6 +38,13 @@ it('populates the layout properly', () => { text: ChartTestUtils.DEFAULT_Y_TITLE, }), }), + annotations: expect.arrayContaining([ + expect.objectContaining({ + text: ChartTestUtils.DEFAULT_CHART_TITLE, + xref: 'x1 domain', + yref: 'y1 domain', + }), + ]), }) ); }); diff --git a/packages/chart/src/FigureChartModel.ts b/packages/chart/src/FigureChartModel.ts index 8b04260398..19ac5c25f1 100644 --- a/packages/chart/src/FigureChartModel.ts +++ b/packages/chart/src/FigureChartModel.ts @@ -13,7 +13,14 @@ import type { } from '@deephaven/jsapi-types'; import Log from '@deephaven/log'; import { Range } from '@deephaven/utils'; -import type { Layout, Data, PlotData } from 'plotly.js'; +import type { + Annotations, + Layout, + Data, + PlotData, + XAxisName, + YAxisName, +} from 'plotly.js'; import type { DateTimeColumnFormatter, Formatter, @@ -134,11 +141,12 @@ class FigureChartModel extends ChartModel { } getDefaultTitle(): string { - if (this.figure.charts.length > 0) { - const chart = this.figure.charts[0]; - return chart.title; + if (this.figure.title != null && this.figure.title.length > 0) { + return this.figure.title; + } + if (this.figure.charts.length === 1) { + return this.figure.charts[0].title ?? ''; } - return ''; } @@ -147,7 +155,7 @@ class FigureChartModel extends ChartModel { this.filterColumnMap.clear(); const { charts } = this.figure; - const axes = ChartUtils.getAllAxes(this.figure); + const axisTypeMap = ChartUtils.getAxisTypeMap(this.figure); const activeSeriesNames: string[] = []; for (let i = 0; i < charts.length; i += 1) { const chart = charts[i]; @@ -155,7 +163,47 @@ class FigureChartModel extends ChartModel { for (let j = 0; j < chart.series.length; j += 1) { const series = chart.series[j]; activeSeriesNames.push(series.name); - this.addSeries(series, axes, chart.showLegend); + this.addSeries(series, axisTypeMap, chart.showLegend); + } + + // Need to add the chart titles as annotations if they are set + const { axes, title } = chart; + if ( + title != null && + title.length > 0 && + (charts.length > 1 || this.figure.title != null) + ) { + const xAxis = axes.find(axis => axis.type === this.dh.plot.AxisType.X); + const yAxis = axes.find(axis => axis.type === this.dh.plot.AxisType.Y); + if (xAxis == null || yAxis == null) { + log.warn( + 'Chart title provided, but unknown how to map to the correct axes for this chart type', + chart + ); + } else { + const xAxisIndex = + (axisTypeMap.get(xAxis.type)?.findIndex(a => a === xAxis) ?? 0) + 1; + const yAxisIndex = + (axisTypeMap.get(yAxis.type)?.findIndex(a => a === yAxis) ?? 0) + 1; + + const annotation: Partial = { + align: 'center', + x: 0.5, + y: 1, + yshift: 17, + text: title, + showarrow: false, + + // Typing is incorrect in Plotly for this, as it doesn't seem to be typed for the "domain" part: https://plotly.com/javascript/reference/layout/annotations/#layout-annotations-items-annotation-xref + xref: `x${xAxisIndex} domain` as XAxisName, + yref: `y${yAxisIndex} domain` as YAxisName, + }; + if (this.layout.annotations == null) { + this.layout.annotations = [annotation]; + } else { + this.layout.annotations.push(annotation); + } + } } } @@ -174,12 +222,15 @@ class FigureChartModel extends ChartModel { /** * Add a series to the model * @param series Series object to add - * @param axes All the axis in this figure + * @param axisTypeMap Map of axis type to the axes in this Figure * @param showLegend Whether this series should show the legend or not */ - addSeries(series: Series, axes: Axis[], showLegend: boolean | null): void { + addSeries( + series: Series, + axisTypeMap: AxisTypeMap, + showLegend: boolean | null + ): void { const { dh } = this; - const axisTypeMap: AxisTypeMap = ChartUtils.groupArray(axes, 'type'); const seriesData = this.chartUtils.makeSeriesDataFromSeries( series, @@ -223,12 +274,12 @@ class FigureChartModel extends ChartModel { // We need to debounce adding series so we subscribe to them all in the same tick // This should no longer be necessary after IDS-5049 lands addPendingSeries = debounce(() => { - const axes = ChartUtils.getAllAxes(this.figure); + const axisTypeMap = ChartUtils.getAxisTypeMap(this.figure); const { pendingSeries } = this; for (let i = 0; i < pendingSeries.length; i += 1) { const series = pendingSeries[i]; const chart = this.figure.charts.find(c => c.series.includes(series)); - this.addSeries(series, axes, chart?.showLegend ?? null); + this.addSeries(series, axisTypeMap, chart?.showLegend ?? null); series.subscribe(); // We'll get an update with the data after subscribing