Skip to content

Commit

Permalink
fix: render continuous line/area between non-adjacent points (opensea…
Browse files Browse the repository at this point in the history
…rch-project#833)

When rendering line or area charts with multiple series, we generally collect all the available X values and we automatically fill each series with missing x data points (to cover the most used case where we are rendering pre-aggregated data in Kibana).
This can cause issues when we want to keep the non-consecutive nature of the line/area. This fix removes the x filling for line and area charts in the following case: non-stacked line or area chart, with continuous scale and no fit function.
We apply this fix only to that specific case because: for stacked lines/area we need to clearly compute the stack of each data point on the x-axis, for categorical-ordinal scale (where a line/area chart doesn't always match with the best practice) the x-axis doesn't always describe a continuity within the data.

fix opensearch-project#825

BREAKING CHANGE: when rendering non-stacked line/area charts with a continuous x scale and no fit function,
the line/area between non-consecutive data points will be rendered as a continuous line/area without adding an uncertain dashed line/ semi-transparent area that connects the two, non-adjacent, points.
  • Loading branch information
markov00 authored Sep 29, 2020
1 parent fd8cc2a commit 5222c40
Show file tree
Hide file tree
Showing 16 changed files with 210 additions and 57 deletions.
30 changes: 9 additions & 21 deletions packages/osd-charts/.playground/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,29 +7,19 @@
<meta http-equiv="X-UA-Compatible" content="ie=edge" />
<style>
html,
body {
body,
#root {
background: blanchedalmond !important;
width: 100%;
height: 100%;
padding: 10px;
}

.chart {
background: white;
/*display: inline-block;
position: relative;
*/
width: 500px;
height: 200px;
overflow: auto;
}

.testing {
background: aquamarine;
position: relative;
width: 100%;
overflow: auto;
}

.page {
padding: 10px;
background: white;
width: 800px;
height: 500px;
}

label {
Expand All @@ -39,9 +29,7 @@
</head>

<body>
<div class="page">
<div id="root"></div>
</div>
<div id="root"></div>
<script src="bundle.js" type="text/javascript"></script>
</body>
</html>
16 changes: 7 additions & 9 deletions packages/osd-charts/.playground/playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,12 @@

import React from 'react';

import { Example } from '../stories/sunburst/15_single_sunburst';
import { Example } from '../stories/bar/1_basic';

export class Playground extends React.Component {
render() {
return (
<div className="testing">
<div className="chart">{Example()}</div>
</div>
);
}
export function Playground() {
return (
<div className="chart">
<Example />
</div>
);
}
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
13 changes: 13 additions & 0 deletions packages/osd-charts/integration/tests/area_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,4 +50,17 @@ describe('Area series stories', () => {
});
});
});
describe('Non-Stacked Linear Area with discontinuous data points', () => {
it('with fit', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=false&knob-switch to area=true',
);
});

it('no fit function', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=true&knob-switch to area=true',
);
});
});
});
13 changes: 13 additions & 0 deletions packages/osd-charts/integration/tests/line_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,17 @@ describe('Line series stories', () => {
);
});
});
describe('Non-Stacked Linear Line with discontinuous data points', () => {
it('with fit', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=false&knob-switch to area=',
);
});

it('no fit function', async () => {
await common.expectChartAtUrlToMatchScreenshot(
'http://localhost:9001/?path=/story/line-chart--discontinuous-data-points&knob-enable fit function=true&knob-switch to area=',
);
});
});
});
50 changes: 39 additions & 11 deletions packages/osd-charts/src/chart_types/xy_chart/state/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import { GroupId, SpecId } from '../../../../utils/ids';
import { ColorConfig, Theme } from '../../../../utils/themes/theme';
import { XDomain, YDomain } from '../../domains/types';
import { mergeXDomain } from '../../domains/x_domain';
import { mergeYDomain } from '../../domains/y_domain';
import { mergeYDomain, splitSpecsByGroupId } from '../../domains/y_domain';
import { renderArea, renderBars, renderLine, renderBubble, isDatumFilled } from '../../rendering/rendering';
import { defaultTickFormatter } from '../../utils/axis_utils';
import { fillSeries } from '../../utils/fill_series';
Expand Down Expand Up @@ -131,18 +131,24 @@ export function getCustomSeriesColors(
return updatedCustomSeriesColors;
}

function getLastValues(formattedDataSeries: {
stacked: FormattedDataSeries[];
nonStacked: FormattedDataSeries[];
}): Map<SeriesKey, LastValues> {
function getLastValues(
formattedDataSeries: {
stacked: FormattedDataSeries[];
nonStacked: FormattedDataSeries[];
},
xDomain: XDomain,
): Map<SeriesKey, LastValues> {
const lastValues = new Map<SeriesKey, LastValues>();

if (xDomain.scaleType === ScaleType.Ordinal) {
return lastValues;
}
// we need to get the latest
formattedDataSeries.stacked.forEach(({ dataSeries, stackMode }) => {
dataSeries.forEach((series) => {
if (series.data.length === 0) {
return;
}

const last = series.data[series.data.length - 1];
if (!last) {
return;
Expand All @@ -151,6 +157,13 @@ function getLastValues(formattedDataSeries: {
return;
}

if (last.x !== xDomain.domain[xDomain.domain.length - 1]) {
// we have a dataset that is not filled with all x values
// and the last value of the series is not the last value for every series
// let's skip it
return;
}

const { y0, y1, initialY0, initialY1 } = last;
const seriesKey = getSeriesKey(series as XYChartSeriesIdentifier);

Expand Down Expand Up @@ -178,6 +191,13 @@ function getLastValues(formattedDataSeries: {
return;
}

if (last.x !== xDomain.domain[xDomain.domain.length - 1]) {
// we have a dataset that is not filled with all x values
// and the last value of the series is not the last value for every series
// let's skip it
return;
}

const { initialY1, initialY0 } = last;
const seriesKey = getSeriesKey(series as XYChartSeriesIdentifier);

Expand All @@ -194,6 +214,7 @@ function getLastValues(formattedDataSeries: {
* @param customXDomain if specified in <Settings />, the custom X domain
* @param deselectedDataSeries is optional; if not supplied,
* @param customXDomain is optional; if not supplied,
* @param orderOrdinalBinsBy
* @param enableVislibSeriesSort is optional; if not specified in <Settings />,
* then all series will be factored into computations. Otherwise, selectedDataSeries
* is used to restrict the computation for just the selected series
Expand All @@ -215,26 +236,33 @@ export function computeSeriesDomains(
enableVislibSeriesSort,
);
// compute the x domain merging any custom domain
const specsArray = [...seriesSpecs.values()];
const xDomain = mergeXDomain(specsArray, xValues, customXDomain, fallbackScale);
const xDomain = mergeXDomain(seriesSpecs, xValues, customXDomain, fallbackScale);

const specsByGroupIds = splitSpecsByGroupId(seriesSpecs);

// fill series with missing x values
const filledDataSeriesBySpecId = fillSeries(dataSeriesBySpecId, xValues);
const filledDataSeriesBySpecId = fillSeries(
dataSeriesBySpecId,
xValues,
seriesSpecs,
xDomain.scaleType,
specsByGroupIds,
);

const formattedDataSeries = getFormattedDataseries(
specsArray,
filledDataSeriesBySpecId,
xValues,
xDomain.scaleType,
seriesSpecs,
specsByGroupIds,
);

// let's compute the yDomain after computing all stacked values
const yDomain = mergeYDomain(formattedDataSeries, seriesSpecs, customYDomainsByGroupId);

// we need to get the last values from the formatted dataseries
// because we change the format if we are on percentage mode
const lastValues = xDomain.scaleType !== ScaleType.Ordinal ? getLastValues(formattedDataSeries) : new Map();
const lastValues = getLastValues(formattedDataSeries, xDomain);
const updatedSeriesCollection = new Map<SeriesKey, SeriesCollectionValue>();
seriesCollection.forEach((value, key) => {
const lastValue = lastValues.get(key);
Expand Down
37 changes: 33 additions & 4 deletions packages/osd-charts/src/chart_types/xy_chart/utils/fill_series.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,47 @@
* specific language governing permissions and limitations
* under the License.
*/
import { SpecId } from '../../../utils/ids';
import { ScaleType } from '../../../scales/constants';
import { SpecId, GroupId } from '../../../utils/ids';
import { YBasicSeriesSpec } from '../domains/y_domain';
import { getSpecsById } from '../state/utils/spec';
import { DataSeries } from './series';
import { SeriesSpecs, StackMode, BasicSeriesSpec, isLineSeriesSpec, isAreaSeriesSpec } from './specs';

/**
* Fill missing x values in all data series
* @param series
* @param xValues
* @internal
*/
export function fillSeries(
series: Map<SpecId, DataSeries[]>,
xValues: Set<string | number>,
seriesSpecs: SeriesSpecs,
groupScaleType: ScaleType,
specsByGroupIds: Map<
GroupId,
{
stackMode: StackMode | undefined;
stacked: YBasicSeriesSpec[];
nonStacked: YBasicSeriesSpec[];
}
>,
): Map<SpecId, DataSeries[]> {
const sortedXValues = [...xValues.values()];
const filledSeries: Map<SpecId, DataSeries[]> = new Map();
series.forEach((dataSeries, key) => {
const spec = getSpecsById(seriesSpecs, key);
if (!spec) {
return;
}
const group = specsByGroupIds.get(spec.groupId);
if (!group) {
return;
}
const isStacked = Boolean(group.stacked.find(({ id }) => id === key));
const noFillRequired = isXFillNotRequired(spec, groupScaleType, isStacked);

const filledDataSeries = dataSeries.map(({ data, ...rest }) => {
if (data.length === xValues.size) {
if (data.length === xValues.size || noFillRequired) {
return {
...rest,
data,
Expand Down Expand Up @@ -74,3 +97,9 @@ export function fillSeries(
});
return filledSeries;
}

function isXFillNotRequired(spec: BasicSeriesSpec, groupScaleType: ScaleType, isStacked: boolean) {
const onlyNoFitAreaLine = (isAreaSeriesSpec(spec) || isLineSeriesSpec(spec)) && !spec.fit;
const onlyContinuous = groupScaleType === ScaleType.Linear || groupScaleType === ScaleType.Time;
return onlyNoFitAreaLine && onlyContinuous && !isStacked;
}
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,10 @@ describe('Non-Stacked Series Utils', () => {
} = computeSeriesDomainsSelector(store.getState());

expect(dataSeries.length).toBe(2);
expect(dataSeries[0].data.length).toBe(4);
expect(dataSeries[1].data.length).toBe(4);
// this because linear non stacked area/lines doesn't fill up the dataset
// with missing x data points
expect(dataSeries[0].data.length).toBe(3);
expect(dataSeries[1].data.length).toBe(2);

expect(dataSeries[0].data[0]).toMatchObject({
initialY0: null,
Expand All @@ -245,7 +247,7 @@ describe('Non-Stacked Series Utils', () => {
y1: 2,
mark: null,
});
expect(dataSeries[0].data[3]).toMatchObject({
expect(dataSeries[0].data[2]).toMatchObject({
initialY0: null,
initialY1: 4,
x: 4,
Expand All @@ -261,7 +263,7 @@ describe('Non-Stacked Series Utils', () => {
y1: 21,
mark: null,
});
expect(dataSeries[1].data[2]).toMatchObject({
expect(dataSeries[1].data[1]).toMatchObject({
initialY0: null,
initialY1: 23,
x: 3,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { AccessorFn } from '../../../utils/accessor';
import { Position } from '../../../utils/commons';
import * as TestDataset from '../../../utils/data_samples/test_dataset';
import { ColorConfig } from '../../../utils/themes/theme';
import { splitSpecsByGroupId } from '../domains/y_domain';
import { computeSeriesDomainsSelector } from '../state/selectors/compute_series_domains';
import {
SeriesCollectionValue,
Expand Down Expand Up @@ -485,12 +486,14 @@ describe('Series', () => {
};
const xValues = new Set([0, 1, 2, 3]);
const splittedDataSeries = getDataSeriesBySpecId([spec1, spec2]);
const specsByGroupIds = splitSpecsByGroupId([spec1, spec2]);

const stackedDataSeries = getFormattedDataseries(
[spec1, spec2],
splittedDataSeries.dataSeriesBySpecId,
xValues,
ScaleType.Linear,
[],
[spec1, spec2],
specsByGroupIds,
);
expect(stackedDataSeries.stacked).toMatchSnapshot();
});
Expand Down
16 changes: 10 additions & 6 deletions packages/osd-charts/src/chart_types/xy_chart/utils/series.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import { Datum, Color } from '../../../utils/commons';
import { GroupId, SpecId } from '../../../utils/ids';
import { Logger } from '../../../utils/logger';
import { ColorConfig } from '../../../utils/themes/theme';
import { splitSpecsByGroupId, YBasicSeriesSpec } from '../domains/y_domain';
import { YBasicSeriesSpec } from '../domains/y_domain';
import { LastValues } from '../state/utils/types';
import { applyFitFunctionToDataSeries } from './fit_function_utils';
import { BasicSeriesSpec, SeriesTypes, SeriesSpecs, SeriesNameConfigOptions, StackMode } from './specs';
Expand Down Expand Up @@ -347,18 +347,22 @@ const getSortedDataSeries = (

/** @internal */
export function getFormattedDataseries(
specs: YBasicSeriesSpec[],
availableDataSeries: Map<SpecId, DataSeries[]>,
xValues: Set<string | number>,
xScaleType: ScaleType,
seriesSpecs: SeriesSpecs,
specsByGroupIdsEntries: Map<
GroupId,
{
stackMode: StackMode | undefined;
stacked: YBasicSeriesSpec[];
nonStacked: YBasicSeriesSpec[];
}
>,
): {
stacked: FormattedDataSeries[];
nonStacked: FormattedDataSeries[];
} {
const specsByGroupIds = splitSpecsByGroupId(specs);
const specsByGroupIdsEntries = [...specsByGroupIds.entries()];

const stackedFormattedDataSeries: {
groupId: GroupId;
dataSeries: DataSeries[];
Expand All @@ -371,7 +375,7 @@ export function getFormattedDataseries(
counts: DataSeriesCounts;
}[] = [];

specsByGroupIdsEntries.forEach(([groupId, groupSpecs]) => {
[...specsByGroupIdsEntries.entries()].forEach(([groupId, groupSpecs]) => {
const { stackMode } = groupSpecs;
// format stacked data series
const stackedDataSeries = getDataSeriesBySpecGroup(groupSpecs.stacked, availableDataSeries);
Expand Down
Loading

0 comments on commit 5222c40

Please sign in to comment.