Skip to content

Commit

Permalink
[FIX] chart: fix misalignment with trend line axis
Browse files Browse the repository at this point in the history 
Task Description

When inserting a trend line on a line chart, depending on the data
we sometimes have a misplaced trend line axis giving an alignment
issue between the trend line and the original plot. For example,
if you have these data :
A1 : A, B1 : 1
A2 : 2, B2 : 2
A3 : 3, B3 : 3
A4 : 4, B4 : 4
A5 : 5, B5 : 5
A6 : 6, B6 : 6
A7 : 7, B7 : 7
A8 : 8, B8 : 8

When you try to insert a linear trend line, the line won't be
aligned to the original line chart. This PR aims to fix this by
changing the labels and axis of the trend line.

Related Task

closes #5282

Task: 4251695
Signed-off-by: Rémi Rahir (rar) <[email protected]>
  • Loading branch information
@anhe-odoo
anhe-odoo committed Jan 6, 2025
1 parent 93eb716 commit aebd38c
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 125 deletions.
7 changes: 4 additions & 3 deletions src/helpers/figures/charts/chart_common.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -490,13 +490,14 @@ export function getTrendDatasetForBarChart(
if (!newValues.length) {
return;
}
return getFullTrendingLineDataSet(dataset, config, newValues);
return getFullTrendingLineDataSet(dataset, config, newValues, newLabels);
}

export function getFullTrendingLineDataSet(
dataset: ChartDataset<"line" | "bar">,
config: TrendConfiguration,
data: number[]
data: number[],
labels: number[]
) {
const defaultBorderColor = colorToRGBA(dataset.backgroundColor as Color);
defaultBorderColor.a = 1;
Expand All @@ -508,7 +509,7 @@ export function getFullTrendingLineDataSet(
xAxisID: TREND_LINE_XAXIS_ID,
yAxisID: dataset.yAxisID,
label: dataset.label ? _t("Trend line for %s", dataset.label) : "",
data,
data: data.map((v, i) => ({ x: labels[i], y: v })),
order: -1,
showLine: true,
pointRadius: 0,
Expand Down
37 changes: 22 additions & 15 deletions src/helpers/figures/charts/chart_common_line_scatter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,12 @@ export function getTrendDatasetForLineChart(
}
const numberOfStep = 5 * labels.length;
const step = (xmax - xmin) / numberOfStep;
const newLabels = range(xmin, xmax + step / 2, step);
const newValues = interpolateData(config, filteredValues, filteredLabels, newLabels);
if (!newValues.length) {
const trendLabels = range(xmin, xmax + step / 2, step);
const trendValues = interpolateData(config, filteredValues, filteredLabels, trendLabels);
if (!trendValues.length) {
return;
}
return getFullTrendingLineDataSet(dataset, config, newValues);
return getFullTrendingLineDataSet(dataset, config, trendValues, trendLabels);
}

export function createLineOrScatterChartRuntime(
Expand Down Expand Up @@ -314,11 +314,15 @@ export function createLineOrScatterChartRuntime(
config.options.scales!.x!.ticks!.callback = (value) =>
formatValue(value, { format: labelFormat, locale });
config.options.plugins!.tooltip!.callbacks!.label = (tooltipItem) => {
const dataSetPoint = dataSetsValues[tooltipItem.datasetIndex!].data![tooltipItem.dataIndex!];
let label: string | number =
tooltipItem.dataset.xAxisID === TREND_LINE_XAXIS_ID
? ""
: tooltipItem.label || labelValues.values[tooltipItem.dataIndex!];
let dataSetPoint: number;
let label: string | number;
if (tooltipItem.dataset.xAxisID === TREND_LINE_XAXIS_ID) {
dataSetPoint = dataSetsValues[tooltipItem.datasetIndex!].data![tooltipItem.dataIndex!].y;
label = "";
} else {
dataSetPoint = dataSetsValues[tooltipItem.datasetIndex!].data![tooltipItem.dataIndex!];
label = labelValues.values[tooltipItem.dataIndex!];
}
if (isNumber(label, locale)) {
label = toNumber(label, locale);
}
Expand Down Expand Up @@ -397,16 +401,19 @@ export function createLineOrScatterChartRuntime(
}
}
if (trendDatasets.length) {
/* We add a second x axis here to draw the trend lines, with the labels length being
* set so that the second axis points match the classical x axis
*/
config.options.scales[TREND_LINE_XAXIS_ID] = {
...xAxis,
type: "category",
labels: range(0, maxLength).map((x) => x.toString()),
offset: false,
display: false,
};
if (axisType === "category") {
/* We add a second x axis here to draw the trend lines, with the labels length being
* set so that the second axis points match the classical x axis
*/
config.options.scales[TREND_LINE_XAXIS_ID]["labels"] = range(0, maxLength).map((x) =>
x.toString()
);
config.options.scales[TREND_LINE_XAXIS_ID]["offset"] = false;
}
/* These datasets must be inserted after the original datasets to ensure the way we
* distinguish the originals and trendLine datasets after
*/
Expand Down
147 changes: 40 additions & 107 deletions tests/figures/chart/chart_plugin.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ChartType as ChartJSType, TooltipItem } from "chart.js";
import { ChartType as ChartJSType, Point, TooltipItem } from "chart.js";
import { CommandResult, Model } from "../../../src";
import { ChartDefinition } from "../../../src/types";
import {
Expand Down Expand Up @@ -49,7 +49,8 @@ import {

import { ChartTerms } from "../../../src/components/translations_terms";
import { FIGURE_ID_SPLITTER, MAX_CHAR_LABEL } from "../../../src/constants";
import { range, toZone, zoneToXc } from "../../../src/helpers";
import { toNumber } from "../../../src/functions/helpers";
import { toZone, zoneToXc } from "../../../src/helpers";
import { BarChart } from "../../../src/helpers/figures/charts";
import { ChartPlugin } from "../../../src/plugins/core";
import { ScatterChartRuntime } from "../../../src/types/chart/scatter_chart";
Expand Down Expand Up @@ -3075,47 +3076,6 @@ test("trend line dataset are put after original dataset in the runtime", async (
expect(datasets[1]).toMatchObject({ label: "serie_2" });
});

test("trend line with time axis use time values as labels", () => {
setFormat(model, "C2:C5", "m/d/yyyy");
createChart(
model,
{
type: "line",
dataSets: [{ dataRange: "B2:B5", trend: { display: true, type: "polynomial", order: 2 } }],
labelRange: "C2:C5",
labelsAsText: false,
},
"1"
);
let config = getChartConfiguration(model, "1");
expect(config.options?.scales?.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 16).map((v) => v.toString()),
});
});

test("trend line with categorical axis use values as labels", () => {
createChart(
model,
{
type: "line",
dataSets: [{ dataRange: "B2:B5", trend: { display: true, type: "polynomial", order: 2 } }],
labelRange: "C2:C5",
labelsAsText: false,
},
"1"
);
const config = getChartConfiguration(model, "1");
expect(config.options?.scales?.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 16).map((v) => v.toString()),
});
});

describe("trending line", () => {
beforeEach(() => {
setGrid(model, {
Expand All @@ -3142,59 +3102,42 @@ describe("trending line", () => {
"1"
);
});

test("trend line works with numerical values as labels", () => {
const config = getChartConfiguration(model, "1");
expect(config.options.scales.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 26).map((v) => v.toString()),
});
const runtime = model.getters.getChartRuntime("1") as LineChartRuntime;
const step = (6 - 1) / 25;
const data = runtime.chartJsConfig.data.datasets[1].data;
const data = getChartConfiguration(model, "1").data.datasets[1].data;
for (let i = 0; i < data.length; i++) {
const value = data[i];
const expectedValue = Math.pow(1 + i * step, 2);
expect(value).toBeCloseTo(expectedValue);
const value = data[i] as Point;
const expectedLabel = 1 + i * step;
const expectedValue = Math.pow(expectedLabel, 2);
expect(value.x).toEqual(expectedLabel);
expect(value.y).toBeCloseTo(expectedValue);
}
});

test("trend line works with datetime values as labels", () => {
setFormat(model, "C1:C5", "m/d/yyyy");
mockChart();
const config = getChartConfiguration(model, "1");
expect(config.options.scales.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 26).map((v) => v.toString()),
});
const runtime = model.getters.getChartRuntime("1") as LineChartRuntime;
const step = (6 - 1) / 25;
const data = runtime.chartJsConfig.data.datasets[1].data;
const data = getChartConfiguration(model, "1").data.datasets[1].data;
for (let i = 0; i < data.length; i++) {
const value = data[i];
const expectedValue = Math.pow(1 + i * step, 2);
expect(value).toBeCloseTo(expectedValue);
const value = data[i] as Point;
const expectedLabel = 1 + i * step;
const expectedValue = Math.pow(expectedLabel, 2);
expect(value.x).toEqual(expectedLabel);
expect(value.y).toBeCloseTo(expectedValue);
}
});

test("trend line works with categorical values as labels", () => {
const config = getChartConfiguration(model, "1");
expect(config.options.scales.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 26).map((v) => v.toString()),
});
const runtime = model.getters.getChartRuntime("1") as LineChartRuntime;
const step = (6 - 1) / 25;
const data = runtime.chartJsConfig.data.datasets[1].data;
const data = getChartConfiguration(model, "1").data.datasets[1].data;
for (let i = 0; i < data.length; i++) {
const value = data[i];
const expectedValue = Math.pow(1 + i * step, 2);
expect(value).toBeCloseTo(expectedValue);
const value = data[i] as Point;
const expectedLabel = 1 + i * step;
const expectedValue = Math.pow(expectedLabel, 2);
expect(value.x).toEqual(expectedLabel);
expect(value.y).toBeCloseTo(expectedValue);
}
});

Expand All @@ -3211,20 +3154,14 @@ describe("trending line", () => {
dataSets: [{ dataRange: "B1:B10", trend: { display: true, type: "polynomial", order: 2 } }],
labelRange: "C1:C10",
});
const config = getChartConfiguration(model, "1");
expect(config.options.scales.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 51).map((v) => v.toString()),
});
const runtime = model.getters.getChartRuntime("1") as LineChartRuntime;
const step = (10 - 1) / 50;
const data = runtime.chartJsConfig.data.datasets[1].data;
const data = getChartConfiguration(model, "1").data.datasets[1].data;
for (let i = 0; i < data.length; i++) {
const value = data[i];
const expectedValue = Math.pow(1 + i * step, 2);
expect(value).toBeCloseTo(expectedValue);
const value = data[i] as Point;
const expectedLabel = 1 + i * step;
const expectedValue = Math.pow(expectedLabel, 2);
expect(value.x).toEqual(expectedLabel);
expect(value.y).toBeCloseTo(expectedValue);
}
});

Expand Down Expand Up @@ -3255,21 +3192,15 @@ describe("trending line", () => {
B5: "36",
C5: "1/12/2024",
});
const config = getChartConfiguration(model, "1");
expect(config.options.scales.x1).toMatchObject({
type: "category",
display: false,
offset: false,
labels: range(0, 26).map((v) => v.toString()),
});
const runtime = model.getters.getChartRuntime("1") as LineChartRuntime;
const step = (6 - 1) / 25;
//@ts-ignore
const data = runtime.chartJsConfig.data.datasets[1].data;
const data = getChartConfiguration(model, "1").data.datasets[1].data;
const initialValue = toNumber("1/7/2024", model.getters.getLocale()) - 1;
for (let i = 0; i < data.length; i++) {
const value = data[i];
const expectedValue = Math.pow(1 + i * step, 2);
expect(value).toBeCloseTo(expectedValue);
const value = data[i] as Point;
const expectedLabel = 1 + i * step;
const expectedValue = Math.pow(expectedLabel, 2);
expect(value.x).toEqual(initialValue + expectedLabel);
expect(value.y).toBeCloseTo(expectedValue);
}
});

Expand Down Expand Up @@ -3320,9 +3251,11 @@ describe("trending line", () => {
const data = runtime.chartJsConfig.data.datasets[1]?.data;
const step = (4 - 1) / (data.length - 1);
for (let i = 0; i < data.length; i++) {
const value = data[i];
const expectedValue = Math.pow(1 + i * step, 2);
expect(value).toBeCloseTo(expectedValue);
const value = data[i] as Point;
const expectedLabel = 1 + i * step;
const expectedValue = Math.pow(expectedLabel, 2);
expect(value.x).toEqual(expectedLabel);
expect(value.y).toBeCloseTo(expectedValue);
}
});

Expand Down

0 comments on commit aebd38c

Please sign in to comment.