Skip to content

Commit

Permalink
fix(barSeries): error rendering bars with negative log scale
Browse files Browse the repository at this point in the history
  • Loading branch information
nickofthyme committed Apr 11, 2024
1 parent 98f3e51 commit 30f5861
Show file tree
Hide file tree
Showing 7 changed files with 113 additions and 49 deletions.
24 changes: 24 additions & 0 deletions e2e/tests/test_cases_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,28 @@ test.describe('Test cases stories', () => {
);
});
});

test.describe('Log scales', () => {
test('should correctly render negative values from baseline when banded', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--log-with-negative-values&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Banded=true&knob-Nice y ticks=true&knob-Scale Type=log&knob-Series Type=bar&knob-Show positive data=&knob-Stacked=&knob-logMinLimit=1&knob-Split=',
);
});

test('should correctly render tooltip values for banded bars', async ({ page }) => {
await common.expectChartWithMouseAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--log-with-negative-values&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Banded=true&knob-Nice y ticks=true&knob-Scale Type=log&knob-Series Type=bar&knob-Show positive data=&knob-Stacked=&knob-logMinLimit=1&knob-Split=',
{
top: 240,
right: 240,
},
);
});

test('should correctly render negative values from baseline when stacked', async ({ page }) => {
await common.expectChartAtUrlToMatchScreenshot(page)(
'http://localhost:9001/?path=/story/test-cases--log-with-negative-values&globals=toggles.showHeader:true;toggles.showChartTitle:false;toggles.showChartDescription:false;toggles.showChartBoundary:false;theme:light&knob-Banded=&knob-Nice y ticks=true&knob-Scale Type=log&knob-Series Type=bar&knob-Show positive data=&knob-Stacked=true&knob-logMinLimit=1&knob-Split=true',
);
});
});
});
25 changes: 11 additions & 14 deletions packages/charts/src/chart_types/xy_chart/rendering/bars.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
*/

import { getDatumYValue } from './points';
import { getY0ScaledValueFn, getY1ScaledValueFn } from './utils';
import { Color } from '../../../common/colors';
import { ScaleBand, ScaleContinuous } from '../../../scales';
import { ScaleType } from '../../../scales/constants';
import { TextMeasure } from '../../../utils/bbox/canvas_text_bbox_calculator';
import { clamp, mergePartial } from '../../../utils/common';
import { Dimensions } from '../../../utils/dimensions';
Expand Down Expand Up @@ -43,40 +43,37 @@ export function renderBars(
chartRotation: number,
minBarHeight: number,
color: Color,
isBandedSpec: boolean,
sharedSeriesStyle: BarSeriesStyle,
displayValueSettings?: DisplayValueSpec,
styleAccessor?: BarStyleAccessor,
stackMode?: StackMode,
): BarTuple {
const { fontSize, fontFamily } = sharedSeriesStyle.displayValue;
const initialBarTuple: BarTuple = { barGeometries: [], indexedGeometryMap: new IndexedGeometryMap() } as BarTuple;
const isLogY = yScale.type === ScaleType.Log;
const isInvertedY = yScale.isInverted;
const y1Fn = getY1ScaledValueFn(yScale);
const y0Fn = getY0ScaledValueFn(yScale);

return dataSeries.data.reduce((barTuple: BarTuple, datum) => {
const xScaled = xScale.scale(datum.x);
if (!xScale.isValueInDomain(datum.x) || Number.isNaN(xScaled)) {
return barTuple; // don't create a bar if not within the xScale domain
}
const { barGeometries, indexedGeometryMap } = barTuple;
const { y0, y1, initialY1, filled } = datum;
const rawY = isLogY && (y1 === 0 || y1 === null) ? yScale.range[0] : yScale.scale(y1);
const { y1, initialY1, filled } = datum;

const y0Scaled = isLogY
? y0 === 0 || y0 === null
? yScale.range[isInvertedY ? 1 : 0]
: yScale.scale(y0)
: yScale.scale(y0 === null ? 0 : y0);
const y1Scaled = y1Fn(datum);
const y0Scaled = y0Fn(datum);

const finiteHeight = y0Scaled - rawY || 0;
const finiteHeight = y0Scaled - y1Scaled || 0;
const absHeight = Math.abs(finiteHeight);
const height = absHeight === 0 ? absHeight : Math.max(minBarHeight, absHeight); // extend nonzero bars
const heightExtension = height - absHeight;
const isUpsideDown = finiteHeight < 0;
const finiteY = Number.isNaN(y0Scaled + rawY) ? 0 : rawY;
const finiteY = Number.isNaN(y0Scaled + y1Scaled) ? 0 : y1Scaled;
const y = isUpsideDown ? finiteY - height + heightExtension : finiteY - heightExtension;

const seriesIdentifier: XYChartSeriesIdentifier = getSeriesIdentifierFromDataSeries(dataSeries);

const seriesStyle = getBarStyleOverrides(datum, seriesIdentifier, sharedSeriesStyle, styleAccessor);

const maxPixelWidth = clamp(seriesStyle.rect.widthRatio ?? 1, 0, 1) * xScale.bandwidth;
Expand All @@ -85,7 +82,7 @@ export function renderBars(
const width = clamp(seriesStyle.rect.widthPixel ?? xScale.bandwidth, minPixelWidth, maxPixelWidth);
const x = xScaled + xScale.bandwidth * orderIndex + xScale.bandwidth / 2 - width / 2;

const y1Value = getDatumYValue(datum, false, false, stackMode);
const y1Value = getDatumYValue(datum, false, isBandedSpec, stackMode);
const formattedDisplayValue = displayValueSettings?.valueFormatter?.(y1Value);

// only show displayValue for even bars if showOverlappingValue
Expand Down
14 changes: 7 additions & 7 deletions packages/charts/src/chart_types/xy_chart/rendering/points.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export function renderPoints(
color: Color,
pointStyle: PointStyle,
isolatedPointThemeStyle: PointStyle,
isBandChart: boolean,
isBandedSpec: boolean,
markSizeOptions: MarkSizeOptions,
useSpatialIndex: boolean,
allowIsolated: boolean,
Expand Down Expand Up @@ -74,12 +74,12 @@ export function renderPoints(
if (Number.isNaN(x)) return acc;

const points: PointGeometry[] = [];
const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = isBandChart ? ['y0', 'y1'] : ['y1'];
const yDatumKeyNames: Array<keyof Omit<FilledValues, 'x'>> = isBandedSpec ? ['y0', 'y1'] : ['y1'];

yDatumKeyNames.forEach((yDatumKeyName, keyIndex) => {
const valueAccessor = getYDatumValueFn(yDatumKeyName);
const y = yDatumKeyName === 'y1' ? y1Fn(datum) : y0Fn(datum);
const originalY = getDatumYValue(datum, keyIndex === 0, isBandChart, dataSeries.stackMode);
const originalY = getDatumYValue(datum, keyIndex === 0, isBandedSpec, dataSeries.stackMode);
const seriesIdentifier: XYChartSeriesIdentifier = getSeriesIdentifierFromDataSeries(dataSeries);
const styleOverrides = getPointStyleOverrides(datum, seriesIdentifier, styleAccessor);
const style = buildPointGeometryStyles(color, pointStyle, styleOverrides);
Expand All @@ -102,7 +102,7 @@ export function renderPoints(
x: xValue,
y: originalY,
mark,
accessor: isBandChart && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
accessor: isBandedSpec && keyIndex === 0 ? BandedAccessorType.Y0 : BandedAccessorType.Y1,
datum: datum.datum,
},
transform: {
Expand Down Expand Up @@ -157,17 +157,17 @@ export function getPointStyleOverrides(
* Get the original/initial Y value from the datum
* @param datum a DataSeriesDatum
* @param lookingForY0 if we are interested in the y0 value, false for y1
* @param isBandChart if the chart is a band chart
* @param isBandedSpec if the chart is a band chart
* @param stackMode an optional stack mode
* @internal
*/
export function getDatumYValue(
{ y1, y0, initialY1, initialY0 }: DataSeriesDatum,
lookingForY0: boolean,
isBandChart: boolean,
isBandedSpec: boolean,
stackMode?: StackMode,
) {
if (isBandChart) {
if (isBandedSpec) {
// on band stacked charts in percentage mode, the values I'm looking for are the percentage value
// that are already computed and available on y0 and y1
// in all other cases for band charts, I want to get back the original/initial value of y0 and y1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ function renderGeometries(
chartRotation,
spec.minBarHeight ?? 0,
color,
isBandedSpec(spec),
barSeriesStyle,
displayValueSettings,
spec.styleAccessor,
Expand Down
30 changes: 2 additions & 28 deletions storybook/stories/scales/7_log_scale_options.story.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,7 @@ import { boolean, number, select } from '@storybook/addon-knobs';
import { range } from 'lodash';
import React from 'react';

import {
Chart,
Axis,
LineSeries,
Position,
ScaleType,
Settings,
AreaSeries,
YDomainBase,
LogScaleOptions,
} from '@elastic/charts';
import { Chart, Axis, Position, ScaleType, Settings, YDomainBase, LogScaleOptions } from '@elastic/charts';

import { ChartsStory } from '../../types';
import { useBaseTheme } from '../../use_base_theme';
Expand Down Expand Up @@ -83,21 +73,6 @@ const getDataValue = (type: string, v: number, i: number, length: number) => {
}
};

const seriesMap = {
line: LineSeries,
area: AreaSeries,
};

const getSeriesType = () =>
select<keyof typeof seriesMap>(
'Series Type',
{
Line: 'line',
Area: 'area',
},
'line',
);

const getInitalData = (rows: number) => {
const quart = Math.round(rows / 4);
return [...range(quart, -quart, -1), ...range(-quart, quart + 1, 1)];
Expand All @@ -118,9 +93,8 @@ export const Example: ChartsStory = (_, { title, description }) => {
const yLogKnobs = getLogKnobs(false);
const xLogKnobs = getLogKnobs(true);
const data = getData(rows, yLogKnobs, xLogKnobs);
const type = getSeriesType();
const [Series] = customKnobs.enum.xySeries('Series Type', 'line', { exclude: ['bubble', 'bar'] });
const curve = customKnobs.enum.curve('Curve type');
const Series = seriesMap[type];

return (
<Chart title={title} description={description}>
Expand Down
67 changes: 67 additions & 0 deletions storybook/stories/test_cases/12_log_with_negative_values.story.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { boolean, number } from '@storybook/addon-knobs';
import numeral from 'numeral';
import React from 'react';

import { Axis, Chart, Position, ScaleType, Settings } from '@elastic/charts';
import { getRandomNumberGenerator } from '@elastic/charts/src/mocks/utils';

import { ChartsStory } from '../../types';
import { useBaseTheme } from '../../use_base_theme';
import { customKnobs } from '../utils/knobs';

const rng = getRandomNumberGenerator();

const data = new Array(20).fill(1).flatMap((_, x) =>
['A', 'B', 'C'].map((g) => {
const y1 = rng(30, 100);
const y0 = rng(0.01, 20);
return {
x,
g,
y1Pos: y1,
y1Neg: -y1,
y0Pos: y0,
y0Neg: -y0,
};
}),
);

export const Example: ChartsStory = (_, { title, description }) => {
const yScaleType = customKnobs.enum.scaleType('Scale Type', ScaleType.Log, { include: ['Linear', 'Log'] });
const [Series] = customKnobs.enum.xySeries('Series Type', 'bar', { exclude: ['bubble'] });
const logMinLimit = number('logMinLimit', 1, { min: 0 });
const yNice = boolean('Nice y ticks', false);
const banded = boolean('Banded', false);
const split = boolean('Split', false);
const stacked = boolean('Stacked', true);
const showPosData = boolean('Show positive data', false);

return (
<Chart title={title} description={description}>
<Settings baseTheme={useBaseTheme()} />
<Axis id="bottom" title="x" position={Position.Bottom} ticks={20} />
<Axis id="left" title="y" position={Position.Left} domain={{ min: NaN, max: NaN, logMinLimit }} />
<Series
id="lines"
xScaleType={ScaleType.Linear}
yScaleType={yScaleType}
tickFormat={(d: number) => numeral(d).format('0.[0]')}
data={split ? data : data.filter(({ g }) => g === 'A')}
yNice={yNice}
xAccessor="x"
yAccessors={showPosData ? ['y1Pos'] : ['y1Neg']}
y0Accessors={!banded ? [] : showPosData ? ['y0Pos'] : ['y0Neg']}
splitSeriesAccessors={split ? ['g'] : []}
stackAccessors={stacked ? ['g'] : []}
/>
</Chart>
);
};
1 change: 1 addition & 0 deletions storybook/stories/test_cases/test_cases.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,4 @@ export { Example as duplicateLabelsInPartitionLegend } from './9_duplicate_label
export { Example as highlighterZIndex } from './10_highlighter_z_index.story';
export { Example as domainEdges } from './21_domain_edges.story';
export { Example as startDayOfWeek } from './11_start_day_of_week.story';
export { Example as logWithNegativeValues } from './12_log_with_negative_values.story';

0 comments on commit 30f5861

Please sign in to comment.