Skip to content

Commit

Permalink
fix: figure_title and chart_title were not mapped up correctly (#…
Browse files Browse the repository at this point in the history
…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)
  • Loading branch information
mofojed authored Dec 19, 2023
1 parent ba1d51b commit 73e0b65
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 14 deletions.
4 changes: 3 additions & 1 deletion packages/chart/src/ChartTestUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -127,7 +129,7 @@ class ChartTestUtils {
}

makeFigure({
title = 'Figure',
title = ChartTestUtils.DEFAULT_FIGURE_TITLE,
charts = [this.makeChart()],
rows = 1,
cols = 1,
Expand Down
10 changes: 10 additions & 0 deletions packages/chart/src/ChartUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion packages/chart/src/FigureChartModel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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',
}),
]),
})
);
});
Expand Down
75 changes: 63 additions & 12 deletions packages/chart/src/FigureChartModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 '';
}

Expand All @@ -147,15 +155,55 @@ 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];

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<Annotations> = {
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);
}
}
}
}

Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 73e0b65

Please sign in to comment.