Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(heatmap): snap time bucket to calendar/fixed intervals #1462

Merged
merged 16 commits into from
Nov 9, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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.
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.
7 changes: 7 additions & 0 deletions integration/tests/heatmap_stories.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,11 @@ describe('Heatmap stories', () => {
'http://localhost:9001/?path=/story/heatmap-alpha--categorical&knob-use global min fontSize_labels=false',
);
});

it.each([[2], [3], [4], [5], [6], [7], [8], [9]])('time snap with dataset %i', async (dataset) => {
await page.setViewport({ width: 785, height: 600 });
await common.expectChartAtUrlToMatchScreenshot(
`http://localhost:9001/?path=/story/heatmap-alpha--time-snap&globals=theme:light&knob-dataset=${dataset}`,
);
});
});
13 changes: 7 additions & 6 deletions packages/charts/api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -710,11 +710,14 @@ export const entryKey: ([key]: ArrayEntry) => string;
export const entryValue: ([, value]: ArrayEntry) => ArrayNode;

// @public (undocumented)
export type ESCalendarInterval = {
export interface ESCalendarInterval {
// (undocumented)
type: 'calendar';
// (undocumented)
unit: ESCalendarIntervalUnit;
// (undocumented)
value: number;
};
}

// @public (undocumented)
export type ESCalendarIntervalUnit = 'minute' | 'm' | 'hour' | 'h' | 'day' | 'd' | 'week' | 'w' | 'month' | 'M' | 'quarter' | 'q' | 'year' | 'y';
Expand Down Expand Up @@ -1021,8 +1024,6 @@ export interface HeatmapConfig {
// (undocumented)
maxRowHeight: Pixels;
// (undocumented)
timeZone: string;
// (undocumented)
width: Pixels;
// Warning: (ae-forgotten-export) The symbol "Font" needs to be exported by the entry point index.d.ts
//
Expand Down Expand Up @@ -2461,8 +2462,8 @@ export type YDomainRange = YDomainBase & DomainRange & LogScaleOptions;
// Warnings were encountered during analysis:
//
// src/chart_types/heatmap/layout/types/config_types.ts:19:13 - (ae-forgotten-export) The symbol "SizeRatio" needs to be exported by the entry point index.d.ts
// src/chart_types/heatmap/layout/types/config_types.ts:47:5 - (ae-forgotten-export) The symbol "TextAlign" needs to be exported by the entry point index.d.ts
// src/chart_types/heatmap/layout/types/config_types.ts:48:5 - (ae-forgotten-export) The symbol "TextBaseline" needs to be exported by the entry point index.d.ts
// src/chart_types/heatmap/layout/types/config_types.ts:46:5 - (ae-forgotten-export) The symbol "TextAlign" needs to be exported by the entry point index.d.ts
// src/chart_types/heatmap/layout/types/config_types.ts:47:5 - (ae-forgotten-export) The symbol "TextBaseline" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:137:5 - (ae-forgotten-export) The symbol "TimeMs" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:138:5 - (ae-forgotten-export) The symbol "AnimKeyframe" needs to be exported by the entry point index.d.ts

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ export const config: Config = {
fill: 'gray',
},

timeZone: 'UTC',

xAxisLabel: {
name: 'X Value',
visible: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export interface Config {
maxColumnWidth: Pixels;
// general text config
fontFamily: FontFamily;
timeZone: string;

/**
* Config of the mask over the area outside of the selected cells
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@ import { ScaleContinuous } from '../../../../scales';
import { ScaleType } from '../../../../scales/constants';
import { SettingsSpec } from '../../../../specs';
import { withTextMeasure } from '../../../../utils/bbox/canvas_text_bbox_calculator';
import { addIntervalToTime, timeRange } from '../../../../utils/chrono/elasticsearch';
import { addIntervalToTime } from '../../../../utils/chrono/elasticsearch';
import { clamp } from '../../../../utils/common';
import { Dimensions } from '../../../../utils/dimensions';
import { ContinuousDomain } from '../../../../utils/domain';
import { Logger } from '../../../../utils/logger';
import { Theme } from '../../../../utils/themes/theme';
import { PrimitiveValue } from '../../../partition_chart/layout/utils/group_by_rollup';
Expand Down Expand Up @@ -86,7 +85,7 @@ export function shapeViewModel(
): ShapeViewModel {
const gridStrokeWidth = config.grid.stroke.width ?? 1;

const { table, yValues, xDomain } = heatmapTable;
const { table, yValues, xValues, xNumericExtent } = heatmapTable;

// measure the text width of all rows values to get the grid area width
const boxedYValues = yValues.map<Box & { value: NonNullable<PrimitiveValue> }>((value) => ({
Expand All @@ -100,26 +99,6 @@ export function shapeViewModel(

const yInvertedScale = scaleQuantize<NonNullable<PrimitiveValue>>().domain([0, height]).range(yValues);

const xScaleConfig = spec.xScale;
const timeScale = isTimeXScale(xScaleConfig)
? new ScaleContinuous(
{
type: ScaleType.Time,
domain: xDomain.domain as number[],
range: [0, chartDimensions.width],
nice: false,
},
{
desiredTickCount: estimatedNonOverlappingTickCount(chartDimensions.width, config.xAxisLabel),
timeZone: config.timeZone,
},
)
: null;

const xValues = isTimeXScale(xScaleConfig)
? timeRange((xDomain.domain as ContinuousDomain)[0], (xDomain.domain as ContinuousDomain)[1], xScaleConfig.interval)
: xDomain.domain;

// compute the scale for the columns positions
const xScale = scaleBand<NonNullable<PrimitiveValue>>().domain(xValues).range([0, chartDimensions.width]);

Expand All @@ -141,14 +120,28 @@ export function shapeViewModel(
scaleCallback: (x: any) => number | undefined | null = xScale,
) => (value: any): TextBox => {
return {
text: formatter(value, { timeZone: config.timeZone }),
text: formatter(value, { timeZone: isTimeXScale(spec.xScale) ? spec.xScale.timeZone ?? 'UTC' : 'UTC' }),
value,
...config.xAxisLabel,
x: chartDimensions.left + (scaleCallback(value) || 0),
y: cellHeight * pageSize + config.xAxisLabel.fontSize / 2 + config.xAxisLabel.padding,
};
};

const timeScale = isTimeXScale(spec.xScale)
? new ScaleContinuous(
{
type: ScaleType.Time,
domain: xNumericExtent,
range: [0, chartDimensions.width],
nice: false,
},
{
desiredTickCount: estimatedNonOverlappingTickCount(chartDimensions.width, config.xAxisLabel),
timeZone: spec.xScale.timeZone,
},
)
: null;
monfera marked this conversation as resolved.
Show resolved Hide resolved
// compute the position of each column label
const textXValues: Array<TextBox> = timeScale
? timeScale.ticks().map<TextBox>(getTextValue(config.xAxisLabel.formatter, (x: any) => timeScale.scale(x)))
Expand Down Expand Up @@ -284,8 +277,8 @@ export function shapeViewModel(
const allXValuesInRange: Array<NonNullable<PrimitiveValue>> = getValuesInRange(xValues, startX, endX);
const allYValuesInRange: Array<NonNullable<PrimitiveValue>> = getValuesInRange(yValues, startY, endY);
const invertedXValues: Array<NonNullable<PrimitiveValue>> =
isTimeXScale(xScaleConfig) && typeof endX === 'number'
? [startX, addIntervalToTime(endX, xScaleConfig.interval, xScaleConfig.timeZone)]
isTimeXScale(spec.xScale) && typeof endX === 'number'
? [startX, addIntervalToTime(endX, spec.xScale.interval, spec.xScale.timeZone)]
: [...allXValuesInRange];
const cells: Cell[] = [];

Expand Down Expand Up @@ -365,19 +358,20 @@ export function shapeViewModel(
};

// vertical lines
const xLines = [];
for (let i = 0; i < xValues.length + 1; i++) {
const x = chartDimensions.left + i * cellWidth;
const y1 = chartDimensions.top;
const y2 = cellHeight * pageSize;
xLines.push({ x1: x, y1, x2: x, y2 });
}
const xLines = Array.from({ length: xValues.length + 1 }, (d, i) => ({
x1: chartDimensions.left + i * cellWidth,
x2: chartDimensions.left + i * cellWidth,
y1: chartDimensions.top,
y2: cellHeight * pageSize,
}));
markov00 marked this conversation as resolved.
Show resolved Hide resolved

// horizontal lines
const yLines = [];
for (let i = 0; i < pageSize + 1; i++) {
const y = i * cellHeight;
yLines.push({ x1: chartDimensions.left, y1: y, x2: chartDimensions.width + chartDimensions.left, y2: y });
}
const yLines = Array.from({ length: pageSize + 1 }, (d, i) => ({
x1: chartDimensions.left,
x2: chartDimensions.left + chartDimensions.width,
y1: i * cellHeight,
y2: i * cellHeight,
}));

const cells = Object.values(cellMap);
const tableMinFontSize = cells.reduce((acc, { fontSize }) => Math.min(acc, fontSize), Infinity);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import { getLegendSizeSelector } from '../../../../state/selectors/get_legend_si
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { Position } from '../../../../utils/common';
import { Dimensions } from '../../../../utils/dimensions';
import { XDomain } from '../../../xy_chart/domains/types';
import { HeatmapCellDatum } from '../../layout/viewmodel/viewmodel';
import { getGridHeightParamsSelector } from './get_grid_full_height';
import { getHeatmapConfigSelector } from './get_heatmap_config';
Expand All @@ -25,10 +24,9 @@ import { getXAxisRightOverflow } from './get_x_axis_right_overflow';
/** @internal */
export interface HeatmapTable {
table: Array<HeatmapCellDatum>;
// unique set of column values
xDomain: XDomain;
// unique set of row values
yValues: Array<string | number>;
xValues: Array<string | number>;
xNumericExtent: [number, number];
extent: [number, number];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('Categorical heatmap brush', () => {
onBrushEnd: onBrushEndMock,
}),
MockSeriesSpec.heatmap({
xScaleType: ScaleType.Ordinal,
xScale: { type: ScaleType.Ordinal },
data: [
{ x: 'a', y: 'ya', value: 1 },
{ x: 'b', y: 'ya', value: 2 },
Expand Down Expand Up @@ -81,7 +81,7 @@ describe('Categorical heatmap brush', () => {
describe('Temporal heatmap brush', () => {
let store: Store<GlobalChartState>;
let onBrushEndMock = jest.fn();
const start = DateTime.fromISO('2021-07-01T00:00:00.000Z');
const start = DateTime.fromISO('2021-07-01T00:00:00', { zone: 'Europe/Rome' });
beforeEach(() => {
store = MockStore.default({ width: 300, height: 300, top: 0, left: 0 }, 'chartId');
onBrushEndMock = jest.fn();
Expand All @@ -91,7 +91,7 @@ describe('Temporal heatmap brush', () => {
onBrushEnd: onBrushEndMock,
}),
MockSeriesSpec.heatmap({
xScaleType: ScaleType.Time,
xScale: { type: ScaleType.Time, timeZone: 'Europe/Rome', interval: { type: 'fixed', unit: 'h', value: 24 } },
data: [
{ x: start.toMillis(), y: 'ya', value: 1 },
{ x: start.plus({ days: 1 }).toMillis(), y: 'ya', value: 2 },
Expand Down Expand Up @@ -126,7 +126,7 @@ describe('Temporal heatmap brush', () => {
);
});

it('should brush on the x scale + minInterval', () => {
it('should brush above every cell', () => {
const caller = createOnBrushEndCaller();
store.dispatch(onPointerMove({ x: 50, y: 50 }, 0));
store.dispatch(onMouseDown({ x: 50, y: 50 }, 100));
Expand All @@ -135,9 +135,9 @@ describe('Temporal heatmap brush', () => {
caller(store.getState());
expect(onBrushEndMock).toBeCalledTimes(1);
const brushEvent = onBrushEndMock.mock.calls[0][0];
expect(brushEvent.cells).toHaveLength(6);
expect(brushEvent.cells).toHaveLength(9);
// it covers from the beginning of the cell to the end of the next cell
expect(brushEvent.x).toEqual([start.toMillis(), start.plus({ days: 2 }).toMillis()]);
expect(brushEvent.x).toEqual([start.toMillis(), start.plus({ days: 3 }).toMillis()]);
expect(brushEvent.y).toEqual(['ya', 'yb', 'yc']);
});
it('should brush on the x scale + minInterval on a single cell', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,14 @@
* Side Public License, v 1.
*/

import { extent } from 'd3-array';

import { getPredicateFn } from '../../../../common/predicate';
import { ScaleType } from '../../../../scales/constants';
import { createCustomCachedSelector } from '../../../../state/create_selector';
import { getSettingsSpecSelector } from '../../../../state/selectors/get_settings_specs';
import { getAccessorValue } from '../../../../utils/accessor';
import { mergeXDomain } from '../../../xy_chart/domains/x_domain';
import { getXNiceFromSpec, getXScaleTypeFromSpec } from '../../../xy_chart/scales/get_api_scales';
import { X_SCALE_DEFAULT } from '../../specs/scale_defaults';
import { addIntervalToTime, timeRange } from '../../../../utils/chrono/elasticsearch';
import { HeatmapTable } from './compute_chart_dimensions';
import { getHeatmapSpecSelector } from './get_heatmap_spec';

Expand All @@ -27,7 +27,7 @@ export const getHeatmapTableSelector = createCustomCachedSelector(
const { data, valueAccessor, xAccessor, yAccessor, xSortPredicate, ySortPredicate } = spec;
const { xDomain } = settingsSpec;

const resultData = data.reduce(
const resultData = data.reduce<HeatmapTable>(
(acc, curr, index) => {
const x = getAccessorValue(curr, xAccessor);

Expand Down Expand Up @@ -59,26 +59,32 @@ export const getHeatmapTableSelector = createCustomCachedSelector(
xValues: [],
yValues: [],
extent: [+Infinity, -Infinity],
xNumericExtent: [+Infinity, -Infinity],
},
);
if (spec.xScale.type === ScaleType.Time) {
const [xDataMin = NaN, xDataMax = NaN] = extent(resultData.xValues as number[]);
// to correctly compute the time extent from data, we need to add an interval to the max value of the dataset
const dataMaxExtended = xDataMax ? addIntervalToTime(xDataMax, spec.xScale.interval, spec.xScale.timeZone) : NaN;

resultData.xDomain = mergeXDomain(
{
type: getXScaleTypeFromSpec(spec.xScale.type),
nice: getXNiceFromSpec(),
isBandScale: false,
desiredTickCount: X_SCALE_DEFAULT.desiredTickCount,
customDomain: xDomain,
},
resultData.xValues,
);

// sort values by their predicates
if (spec.xScale.type === ScaleType.Ordinal) {
resultData.xDomain.domain.sort(getPredicateFn(xSortPredicate));
const [customMin, customMax] = !Array.isArray(xDomain) ? [xDomain?.min ?? NaN, xDomain?.max ?? NaN] : [NaN, NaN];
const [min, max] = extent([xDataMin, customMin, customMax, dataMaxExtended]);
resultData.xNumericExtent = [min ?? NaN, max ?? NaN];
resultData.xValues =
isFiniteNumber(min) && isFiniteNumber(max)
? timeRange(min, max, spec.xScale.interval, spec.xScale.timeZone)
: [];
} else if (spec.xScale.type === ScaleType.Ordinal) {
resultData.xValues.sort(getPredicateFn(xSortPredicate));
}

// sort Y values by their predicates
resultData.yValues.sort(getPredicateFn(ySortPredicate));

return resultData;
},
);

function isFiniteNumber(value: number | undefined): value is number {
return Number.isFinite(value);
}
Comment on lines +88 to +90
Copy link
Contributor

@monfera monfera Nov 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I hope TS will fix this oversight eventually. As it's currently an abstraction that's leaking from hundreds of self-inflicted wounds, Number.isFinite is one of those things that should narrow types (here, to number if the result is true), and this utility is a nice workaround in the meantime

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know, I was following your comments on the TS repo and I was thinking that, for now, can solve our cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it could be a value: unknown, and it can be put into common or utils, as there are many places where I temporarily used a typeof d === 'number' && Number.isFinite(d) check where isFiniteNumber would be clearer. All places where we do === 'number' is an easy to grep candidate for a future PR. Even the name of this function is perfect 👌

Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('Heatmap picked cells', () => {
[
MockGlobalSpec.settingsNoMargins({ onBrushEnd: onBrushEndMock }),
MockSeriesSpec.heatmap({
xScaleType: ScaleType.Ordinal,
xScale: { type: ScaleType.Ordinal },
data: [
{ x: 'a', y: 'ya', value: 1 },
{ x: 'b', y: 'ya', value: 2 },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,27 @@ import { ScaleType } from '../../../../scales/constants';
import { createCustomCachedSelector } from '../../../../state/create_selector';
import { withTextMeasure } from '../../../../utils/bbox/canvas_text_bbox_calculator';
import { getHeatmapConfigSelector } from './get_heatmap_config';
import { getHeatmapSpecSelector } from './get_heatmap_spec';
import { getHeatmapTableSelector } from './get_heatmap_table';

/**
* @internal
* Gets color scale based on specification and values range.
*/
export const getXAxisRightOverflow = createCustomCachedSelector(
[getHeatmapConfigSelector, getHeatmapTableSelector],
({ xAxisLabel: { fontSize, fontFamily, padding, formatter, width }, timeZone }, { xDomain: { domain, type } }) => {
return type !== ScaleType.Time
[getHeatmapSpecSelector, getHeatmapConfigSelector, getHeatmapTableSelector],
({ xScale }, { xAxisLabel: { fontSize, fontFamily, padding, formatter, width } }, { xNumericExtent }) => {
return xScale.type !== ScaleType.Time
? 0
: typeof width === 'number'
? width / 2
: withTextMeasure((measure) =>
new ScaleContinuous({ type: ScaleType.Time, domain: domain as number[], range: [0, 1] }, { timeZone })
: withTextMeasure((measure) => {
return new ScaleContinuous(
{ type: ScaleType.Time, domain: xNumericExtent, range: [0, 1] },
{ timeZone: xScale.type === ScaleType.Time ? xScale.timeZone : undefined },
)
.ticks()
.reduce((max, n) => Math.max(max, measure(formatter(n), padding, fontSize, fontFamily).width + padding), 0),
) / 2;
.reduce((max, n) => Math.max(max, measure(formatter(n), padding, fontSize, fontFamily).width + padding), 0);
}) / 2;
},
);
2 changes: 1 addition & 1 deletion packages/charts/src/mocks/specs/specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ export class MockSeriesSpec {
},
xAccessor: ({ x }: { x: string | number }) => x,
yAccessor: ({ y }: { y: string | number }) => y,
xScaleType: X_SCALE_DEFAULT.type,
xScale: { type: X_SCALE_DEFAULT.type },
valueAccessor: ({ value }: { value: string | number }) => value,
valueFormatter: (value: number) => `${value}`,
xSortPredicate: Predicate.AlphaAsc,
Expand Down
Loading