Skip to content

Commit

Permalink
fix: graceful scale fallbacks and warnings (opensearch-project#704)
Browse files Browse the repository at this point in the history
  • Loading branch information
nickofthyme authored Jun 15, 2020
1 parent 263ad92 commit fe8e322
Show file tree
Hide file tree
Showing 9 changed files with 183 additions and 37 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,7 @@ class PartitionComponent extends React.Component<PartitionProps> {
indices.forEach((i) => datumIndices.add(i));
}
});
/*
*console.log(
* pickedShapes.map((s) => s.value),
* [...datumIndices.values()],
*);
*/

return pickedShapes; // placeholder
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,61 @@ describe('X Domain', () => {
);
expect(mergedDomain.domain).toEqual([0, 1, 2, 5, 7]);
});

test('Should fallback to ordinal scale if not array of numbers', () => {
const ds1: BasicSeriesSpec = {
chartType: ChartTypes.XYAxis,
specType: SpecTypes.Series,
id: 'ds1',
groupId: 'g1',
seriesType: SeriesTypes.Bar,
xAccessor: 'x',
yAccessors: ['y'],
xScaleType: ScaleType.Linear,
yScaleType: ScaleType.Linear,
yScaleToDataExtent: false,
data: [
{ x: 0, y: 0 },
{ x: 'a', y: 0 },
{ x: 2, y: 0 },
{ x: 5, y: 0 },
],
};
const ds2: BasicSeriesSpec = {
chartType: ChartTypes.XYAxis,
specType: SpecTypes.Series,
id: 'ds2',
groupId: 'g2',
seriesType: SeriesTypes.Bar,
xAccessor: 'x',
yAccessors: ['y'],
xScaleType: ScaleType.Linear,
yScaleType: ScaleType.Linear,
yScaleToDataExtent: false,
data: [
{ x: 0, y: 0 },
{ x: 7, y: 0 },
],
};
const specDataSeries = [ds1, ds2];

const { xValues } = getSplittedSeries(specDataSeries);
const mergedDomain = mergeXDomain(
[
{
seriesType: SeriesTypes.Bar,
xScaleType: ScaleType.Linear,
},
{
seriesType: SeriesTypes.Bar,
xScaleType: ScaleType.Ordinal,
},
],
xValues,
);
expect(mergedDomain.domain).toEqual([0, 'a', 2, 5, 7]);
expect(mergedDomain.scaleType).toEqual(ScaleType.Ordinal);
});
test('Should merge multi bar/line ordinal series correctly', () => {
const ds1: BasicSeriesSpec = {
chartType: ChartTypes.XYAxis,
Expand Down
22 changes: 13 additions & 9 deletions packages/osd-charts/src/chart_types/xy_chart/domains/x_domain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@
*/

import { ScaleType } from '../../../scales/constants';
import { compareByValueAsc, identity, isNumberArray } from '../../../utils/commons';
import { compareByValueAsc, identity } from '../../../utils/commons';
import { computeContinuousDataDomain, computeOrdinalDataDomain, Domain } from '../../../utils/domain';
import { Logger } from '../../../utils/logger';
import { isCompleteBound, isLowerBound, isUpperBound } from '../utils/axis_type_utils';
import { BasicSeriesSpec, DomainRange, SeriesTypes } from '../utils/specs';
import { XDomain } from './types';
Expand All @@ -36,6 +37,7 @@ export function mergeXDomain(
specs: Pick<BasicSeriesSpec, 'seriesType' | 'xScaleType'>[],
xValues: Set<string | number>,
customXDomain?: DomainRange | Domain,
fallbackScale?: ScaleType,
): XDomain {
const mainXScaleType = convertXScaleTypes(specs);
if (!mainXScaleType) {
Expand All @@ -46,7 +48,13 @@ export function mergeXDomain(
let seriesXComputedDomains;
let minInterval = 0;

if (mainXScaleType.scaleType === ScaleType.Ordinal) {
if (mainXScaleType.scaleType === ScaleType.Ordinal || fallbackScale === ScaleType.Ordinal) {
if (mainXScaleType.scaleType !== ScaleType.Ordinal) {
Logger.warn(
`Each X value in a ${mainXScaleType.scaleType} x scale needs be be a number. Using ordinal x scale as fallback.`,
);
}

seriesXComputedDomains = computeOrdinalDataDomain(values, identity, false, true);
if (customXDomain) {
if (Array.isArray(customXDomain)) {
Expand All @@ -58,11 +66,7 @@ export function mergeXDomain(
} else {
seriesXComputedDomains = computeContinuousDataDomain(values, identity, true);
let customMinInterval: undefined | number;
if (!isNumberArray(values)) {
throw new Error(
`Each X value in a ${mainXScaleType.scaleType} x scale needs be be a number. String or objects are not allowed`,
);
}

if (customXDomain) {
if (Array.isArray(customXDomain)) {
throw new TypeError('xDomain for continuous scale should be a DomainRange object, not an array');
Expand Down Expand Up @@ -91,7 +95,7 @@ export function mergeXDomain(
seriesXComputedDomains = [computedDomainMin, customXDomain.max];
}
}
const computedMinInterval = findMinInterval(values);
const computedMinInterval = findMinInterval(values as number[]);
if (customMinInterval != null) {
// Allow greater custom min iff xValues has 1 member.
if (xValues.size > 1 && customMinInterval > computedMinInterval) {
Expand All @@ -107,7 +111,7 @@ export function mergeXDomain(

return {
type: 'xDomain',
scaleType: mainXScaleType.scaleType,
scaleType: fallbackScale ?? mainXScaleType.scaleType,
isBandScale: mainXScaleType.isBandScale,
domain: seriesXComputedDomains,
minInterval,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,13 +180,15 @@ export function computeSeriesDomains(
deselectedDataSeries: SeriesIdentifier[] = [],
customXDomain?: DomainRange | Domain,
): SeriesDomainsAndData {
const { splittedSeries, xValues, seriesCollection } = deselectedDataSeries
? getSplittedSeries(seriesSpecs, deselectedDataSeries)
: getSplittedSeries(seriesSpecs, []);
const {
splittedSeries,
xValues, seriesCollection,
fallbackScale,
} = getSplittedSeries(seriesSpecs, deselectedDataSeries);
const splittedDataSeries = [...splittedSeries.values()];
const specsArray = [...seriesSpecs.values()];

const xDomain = mergeXDomain(specsArray, xValues, customXDomain);
const xDomain = mergeXDomain(specsArray, xValues, customXDomain, fallbackScale);
const yDomain = mergeYDomain(splittedSeries, specsArray, customYDomainsByGroupId);

const formattedDataSeries = getFormattedDataseries(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -683,26 +683,26 @@ describe('Series', () => {
expect(getSortedDataSeriesColorsValuesMap(seriesCollection)).toEqual(undefinedSortedColorValues);
});
test('clean datum shall parse string as number for y values', () => {
let datum = cleanDatum([0, 1, 2], 0, 1, 2);
let datum = cleanDatum([0, 1, 2], 0, 1, [], 2);
expect(datum).toBeDefined();
expect(datum?.y1).toBe(1);
expect(datum?.y0).toBe(2);
datum = cleanDatum([0, '1', 2], 0, 1, 2);
datum = cleanDatum([0, '1', 2], 0, 1, [], 2);
expect(datum).toBeDefined();
expect(datum?.y1).toBe(1);
expect(datum?.y0).toBe(2);

datum = cleanDatum([0, '1', '2'], 0, 1, 2);
datum = cleanDatum([0, '1', '2'], 0, 1, [], 2);
expect(datum).toBeDefined();
expect(datum?.y1).toBe(1);
expect(datum?.y0).toBe(2);

datum = cleanDatum([0, 1, '2'], 0, 1, 2);
datum = cleanDatum([0, 1, '2'], 0, 1, [], 2);
expect(datum).toBeDefined();
expect(datum?.y1).toBe(1);
expect(datum?.y0).toBe(2);

datum = cleanDatum([0, 'invalid', 'invalid'], 0, 1, 2);
datum = cleanDatum([0, 'invalid', 'invalid'], 0, 1, [], 2);
expect(datum).toBeDefined();
expect(datum?.y1).toBe(null);
expect(datum?.y0).toBe(null);
Expand Down
41 changes: 35 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 @@ -23,6 +23,7 @@ import { ColorOverrides } from '../../../state/chart_state';
import { Accessor, AccessorFn, getAccessorValue } from '../../../utils/accessor';
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 { LastValues } from '../state/utils/types';
Expand Down Expand Up @@ -150,6 +151,7 @@ export function splitSeries({
const series = new Map<SeriesKey, RawDataSeries>();
const colorsValues = new Set<string>();
const xValues = new Set<string | number>();
const nonNumericValues: any[] = [];

data.forEach((datum) => {
const splitAccessors = getSplitAccessors(datum, splitSeriesAccessors);
Expand All @@ -164,6 +166,7 @@ export function splitSeries({
datum,
xAccessor,
accessor,
nonNumericValues,
y0Accessors && y0Accessors[index],
markSizeAccessor,
);
Expand All @@ -175,7 +178,14 @@ export function splitSeries({
}
});
} else {
const cleanedDatum = cleanDatum(datum, xAccessor, yAccessors[0], y0Accessors && y0Accessors[0], markSizeAccessor);
const cleanedDatum = cleanDatum(
datum,
xAccessor,
yAccessors[0],
nonNumericValues,
y0Accessors && y0Accessors[0],
markSizeAccessor,
);
if (cleanedDatum !== null && cleanedDatum.x !== null && cleanedDatum.x !== undefined) {
xValues.add(cleanedDatum.x);
const seriesKey = updateSeriesMap(series, splitAccessors, yAccessors[0], cleanedDatum, specId);
Expand All @@ -184,6 +194,12 @@ export function splitSeries({
}
});

if (nonNumericValues.length > 0) {
Logger.warn(`Found non-numeric y value${nonNumericValues.length > 1 ? 's' : ''} in dataset for spec "${specId}"`,
`(${nonNumericValues.map((v) => JSON.stringify(v)).join(', ')})`
);
}

return {
rawDataSeries: [...series.values()],
colorsValues,
Expand Down Expand Up @@ -266,6 +282,7 @@ export function cleanDatum(
datum: Datum,
xAccessor: Accessor | AccessorFn,
yAccessor: Accessor,
nonNumericValues: any[],
y0Accessor?: Accessor,
markSizeAccessor?: Accessor | AccessorFn,
): RawDataSeriesDatum | null {
Expand All @@ -280,20 +297,26 @@ export function cleanDatum(
}

const mark = markSizeAccessor === undefined ? null : getAccessorValue(datum, markSizeAccessor);
const y1 = castToNumber(datum[yAccessor]);
const y1 = castToNumber(datum[yAccessor], nonNumericValues);
const cleanedDatum: RawDataSeriesDatum = { x, y1, datum, y0: null, mark };
if (y0Accessor) {
cleanedDatum.y0 = castToNumber(datum[y0Accessor as keyof typeof datum]);
cleanedDatum.y0 = castToNumber(datum[y0Accessor as keyof typeof datum], nonNumericValues);
}
return cleanedDatum;
}

function castToNumber(value: any): number | null {
function castToNumber(value: any, nonNumericValues: any[]): number | null {
if (value === null || value === undefined) {
return null;
}
const num = Number(value);
return isNaN(num) ? null : num;

if (isNaN(num)) {
nonNumericValues.push(value);

return null;
}
return num;
}

/** @internal */
Expand Down Expand Up @@ -406,10 +429,12 @@ export function getSplittedSeries(
splittedSeries: Map<SpecId, RawDataSeries[]>;
seriesCollection: Map<SeriesKey, SeriesCollectionValue>;
xValues: Set<string | number>;
fallbackScale?: ScaleType;
} {
const splittedSeries = new Map<SpecId, RawDataSeries[]>();
const seriesCollection = new Map<SeriesKey, SeriesCollectionValue>();
const xValues: Set<any> = new Set();
let isNumberArray = true;
let isOrdinalScale = false;
// eslint-disable-next-line no-restricted-syntax
for (const spec of seriesSpecs) {
Expand Down Expand Up @@ -437,6 +462,9 @@ export function getSplittedSeries(

// eslint-disable-next-line no-restricted-syntax
for (const xValue of dataSeries.xValues) {
if (isNumberArray && typeof xValue !== 'number') {
isNumberArray = false;
}
xValues.add(xValue);
}
}
Expand All @@ -445,7 +473,8 @@ export function getSplittedSeries(
splittedSeries,
seriesCollection,
// keep the user order for ordinal scales
xValues: isOrdinalScale ? xValues : new Set([...xValues].sort()),
xValues: (isOrdinalScale || !isNumberArray) ? xValues : new Set([...xValues].sort()),
fallbackScale: (!isOrdinalScale && !isNumberArray) ? ScaleType.Ordinal : undefined,
};
}

Expand Down
4 changes: 2 additions & 2 deletions packages/osd-charts/src/state/chart_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import { Spec, PointerEvent } from '../specs';
import { DEFAULT_SETTINGS_SPEC } from '../specs/constants';
import { Color } from '../utils/commons';
import { Dimensions } from '../utils/dimensions';
import { Logger } from '../utils/logger';
import { Point } from '../utils/point';
import { StateActions } from './actions';
import { CHART_RENDERED } from './actions/chart';
Expand Down Expand Up @@ -408,8 +409,7 @@ function findMainChartType(specs: SpecList): ChartTypes | null {
// https://stackoverflow.com/questions/55012174/why-doesnt-object-keys-return-a-keyof-type-in-typescript
const chartTypes = Object.keys(types).filter((type) => type !== ChartTypes.Global);
if (chartTypes.length > 1) {
// eslint-disable-next-line no-console
console.warn('Multiple chart type on the same configuration');
Logger.warn('Multiple chart type on the same configuration');
return null;
}
return chartTypes[0] as ChartTypes;
Expand Down
5 changes: 0 additions & 5 deletions packages/osd-charts/src/utils/commons.ts
Original file line number Diff line number Diff line change
Expand Up @@ -326,11 +326,6 @@ export function mergePartial<T>(
return getPartialValue<T>(baseClone, partial, additionalPartials);
}

/** @internal */
export function isNumberArray(value: unknown): value is number[] {
return Array.isArray(value) && value.every((element) => typeof element === 'number');
}

/** @internal */
export function getUniqueValues<T>(fullArray: T[], uniqueProperty: keyof T): T[] {
return fullArray.reduce<{
Expand Down
Loading

0 comments on commit fe8e322

Please sign in to comment.