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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e99af4c
Fix the heatmap time discretization
markov00 Nov 4, 2021
4f6e7c5
Update API specs
markov00 Nov 4, 2021
5c16f84
Fix basic heatmap story
markov00 Nov 4, 2021
7c89a57
Fix time story
markov00 Nov 4, 2021
eb00c5d
Align both ESInterval to interfaces
markov00 Nov 4, 2021
bc0fffd
Fix test used types
markov00 Nov 4, 2021
100bf5b
Fix the fixed interval time range
markov00 Nov 5, 2021
296d46e
Filter duplicated x values around DST
markov00 Nov 5, 2021
500f58f
Fix missing timezone in computing the fixedTimeRange
markov00 Nov 5, 2021
7a0d7db
Fix the brush to cover gte:start to lt:end
markov00 Nov 5, 2021
4057868
Fix linting errors
markov00 Nov 5, 2021
f623e80
Use UTC formatting on heatmap time story
markov00 Nov 8, 2021
59882e3
Simplify story data
markov00 Nov 8, 2021
0eb451a
Better types for heatmap scales
markov00 Nov 8, 2021
ad7c7d0
Move back to the original position the timeZone param
markov00 Nov 8, 2021
985e3fc
Update chart API report
markov00 Nov 8, 2021
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
41 changes: 40 additions & 1 deletion packages/charts/api/charts.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,29 @@ export const entryKey: ([key]: ArrayEntry) => string;
// @public (undocumented)
export const entryValue: ([, value]: ArrayEntry) => ArrayNode;

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

// @public (undocumented)
export type ESCalendarIntervalUnit = 'minute' | 'm' | 'hour' | 'h' | 'day' | 'd' | 'week' | 'w' | 'month' | 'M' | 'quarter' | 'q' | 'year' | 'y';
Comment on lines +722 to +723
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily in this PR, we could eventually depend on types in elasticsearch.js as an authoritative source

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, as we have an elasticsearch.ts, I meant, at some point directly devDepending on https://github.com/elastic/elasticsearch-js for these types. This way, if elasticsearch adds a new unit, and we bump the dependency, we'll get automatic static errors saying that we don't cover the new unit option(s)


// @public (undocumented)
export interface ESFixedInterval {
// (undocumented)
type: 'fixed';
// (undocumented)
unit: ESFixedIntervalUnit;
// (undocumented)
value: number;
}

// @public (undocumented)
export type ESFixedIntervalUnit = 'ms' | 's' | 'm' | 'h' | 'd';

// @alpha
export interface ExternalPointerEventsSettings {
tooltip: TooltipPortalSettings<'chart'> & {
Expand Down Expand Up @@ -1036,6 +1059,12 @@ export interface HeatmapConfig {
// @public (undocumented)
export type HeatmapElementEvent = [Cell, SeriesIdentifier];

// @alpha (undocumented)
export interface HeatmapNonTimeScale {
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: A positively stated scale name would work better, if possible. The word "scalar" implies unitlessness but it doesn't look nice next to "Scale", so, maybe, UnitlessScale?

The word "Heatmap" in the scale name represents tight coupling, because it casts the Scale as a property of (subsidiary to) the chart type Heatmap, while they're at least equal, orthogonal things or even, Heatmap is subsidiary to the Scale. Of course I see how you want to rule out logarithmic etc. scales.

Maybe UnitlessHeatmapScale is an OK alternative

Copy link
Member Author

Choose a reason for hiding this comment

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

or I can completely remove that named type and use something like:

xScale:
    | HeatmapTimeScale
    | {
        type: typeof ScaleType.Ordinal | typeof ScaleType.Linear;
      };

wdyt?

with your consideration should also HeatmapTimeScale be renamed to TimeHeatmapScale?

Copy link
Contributor

Choose a reason for hiding this comment

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

Part 1: I'm fine with that too, though in terms of syntax, it combines two types of unioning, where it could be just one, eg. { type: ordinal | linear | time } or xScale: Heatmap | Ordinal | Linear

Part 2: Neither name is something I really prefer, as both of them tightly bind a scale type to a chart type maybe needlessly, though the first option rolls off the tongue more easily to me. But the best would be, if possible, to not have a chart type name in the name of a scale type. So, something like TimeScale, or however we call it in xy already

// (undocumented)
type: typeof ScaleType.Ordinal | typeof ScaleType.Linear;
}

// @alpha (undocumented)
export interface HeatmapSpec extends Spec {
// (undocumented)
Expand All @@ -1062,7 +1091,7 @@ export interface HeatmapSpec extends Spec {
// (undocumented)
xAccessor: Accessor | AccessorFn;
// (undocumented)
xScaleType: SeriesScales['xScaleType'];
xScale: HeatmapTimeScale | HeatmapNonTimeScale;
// (undocumented)
xSortPredicate: Predicate;
// (undocumented)
Expand All @@ -1071,6 +1100,16 @@ export interface HeatmapSpec extends Spec {
ySortPredicate: Predicate;
}

// @alpha (undocumented)
export interface HeatmapTimeScale {
// (undocumented)
interval: ESCalendarInterval | ESFixedInterval;
// (undocumented)
timeZone: string;
// (undocumented)
type: typeof ScaleType.Time;
}

// @public
export const HIERARCHY_ROOT_KEY: Key;

Expand Down
58 changes: 28 additions & 30 deletions packages/charts/src/chart_types/heatmap/layout/viewmodel/viewmodel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,14 @@ import { ScaleContinuous } from '../../../../scales';
import { ScaleType } from '../../../../scales/constants';
import { SettingsSpec } from '../../../../specs';
import { withTextMeasure } from '../../../../utils/bbox/canvas_text_bbox_calculator';
import { snapDateToESInterval } from '../../../../utils/chrono/elasticsearch';
import { clamp, range } from '../../../../utils/common';
import { addIntervalToTime, timeRange } 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';
import { HeatmapSpec } from '../../specs';
import { HeatmapSpec, HeatmapTimeScale } from '../../specs';
import { HeatmapTable } from '../../state/selectors/compute_chart_dimensions';
import { ColorScale } from '../../state/selectors/get_color_scale';
import { GridHeightParams } from '../../state/selectors/get_grid_full_height';
Expand Down Expand Up @@ -100,33 +100,26 @@ export function shapeViewModel(

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

const timeScale =
xDomain.type === ScaleType.Time
? 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 = timeScale
? range(
snapDateToESInterval(
(xDomain.domain as ContinuousDomain)[0],
{ type: 'fixed', unit: 'ms', quantity: xDomain.minInterval },
'start',
),
(xDomain.domain as ContinuousDomain)[1],
xDomain.minInterval,
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 Down Expand Up @@ -291,8 +284,9 @@ 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>> =
timeScale && typeof endX === 'number' ? [startX, endX + xDomain.minInterval] : [...allXValuesInRange];

isTimeXScale(xScaleConfig) && typeof endX === 'number'
? [startX, addIntervalToTime(endX, xScaleConfig.interval, xScaleConfig.timeZone)]
: [...allXValuesInRange];
const cells: Cell[] = [];

allXValuesInRange.forEach((x) => {
Expand Down Expand Up @@ -423,3 +417,7 @@ function getCellKey(x: NonNullable<PrimitiveValue>, y: NonNullable<PrimitiveValu
function isValueHidden(value: number, rangesToHide: Array<[number, number]>) {
return rangesToHide.some(([min, max]) => min <= value && value < max);
}

function isTimeXScale(scale: HeatmapSpec['xScale']): scale is HeatmapTimeScale {
return scale.type === ScaleType.Time;
}
21 changes: 17 additions & 4 deletions packages/charts/src/chart_types/heatmap/specs/heatmap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,11 @@ import { ChartType } from '../..';
import { Color } from '../../../common/colors';
import { Predicate } from '../../../common/predicate';
import { ScaleType } from '../../../scales/constants';
import { SeriesScales, Spec } from '../../../specs';
import { Spec } from '../../../specs';
import { SpecType } from '../../../specs/constants';
import { getConnect, specComponentFactory } from '../../../state/spec_factory';
import { Accessor, AccessorFn } from '../../../utils/accessor';
import { ESCalendarInterval, ESFixedInterval } from '../../../utils/chrono/elasticsearch';
import { Datum, RecursivePartial } from '../../../utils/common';
import { config } from '../layout/config/config';
import { Config } from '../layout/types/config_types';
Expand All @@ -27,7 +28,7 @@ const defaultProps = {
data: [],
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 Expand Up @@ -58,6 +59,18 @@ export interface HeatmapBandsColorScale {
labelFormatter?: (start: number, end: number) => string;
}

/** @alpha */
export interface HeatmapTimeScale {
type: typeof ScaleType.Time;
interval: ESCalendarInterval | ESFixedInterval;
timeZone: string;
}

/** @alpha */
export interface HeatmapNonTimeScale {
type: typeof ScaleType.Ordinal | typeof ScaleType.Linear;
}

/** @alpha */
export interface HeatmapSpec extends Spec {
specType: typeof SpecType.Series;
Expand All @@ -70,7 +83,7 @@ export interface HeatmapSpec extends Spec {
valueFormatter: (value: number) => string;
xSortPredicate: Predicate;
ySortPredicate: Predicate;
xScaleType: SeriesScales['xScaleType'];
xScale: HeatmapTimeScale | HeatmapNonTimeScale;
config: RecursivePartial<Config>;
highlightedData?: { x: Array<string | number>; y: Array<string | number> };
name?: string;
Expand All @@ -90,6 +103,6 @@ export const Heatmap: React.FunctionComponent<
| 'xSortPredicate'
| 'valueFormatter'
| 'config'
| 'xScaleType'
| 'xScale'
>(defaultProps),
);
4 changes: 2 additions & 2 deletions packages/charts/src/chart_types/heatmap/state/selectors/get_heatmap_table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export const getHeatmapTableSelector = createCustomCachedSelector(

resultData.xDomain = mergeXDomain(
{
type: getXScaleTypeFromSpec(spec.xScaleType),
type: getXScaleTypeFromSpec(spec.xScale.type),
nice: getXNiceFromSpec(),
isBandScale: false,
desiredTickCount: X_SCALE_DEFAULT.desiredTickCount,
Expand All @@ -74,7 +74,7 @@ export const getHeatmapTableSelector = createCustomCachedSelector(
);

// sort values by their predicates
if (spec.xScaleType === ScaleType.Ordinal) {
if (spec.xScale.type === ScaleType.Ordinal) {
resultData.xDomain.domain.sort(getPredicateFn(xSortPredicate));
}
resultData.yValues.sort(getPredicateFn(ySortPredicate));
Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/specs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,4 @@ export * from './xy_chart/utils/specs';

export { Partition } from './partition_chart/specs';

export { Heatmap, HeatmapSpec } from './heatmap/specs';
export { Heatmap, HeatmapSpec, HeatmapTimeScale, HeatmapNonTimeScale } from './heatmap/specs';
7 changes: 7 additions & 0 deletions packages/charts/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,10 @@ export { Pixels, Ratio } from './common/geometry';
export { AdditiveNumber } from './utils/accessor';
export { FontStyle, FONT_STYLES } from './common/text_utils';
export { Color } from './common/colors';

export {
ESCalendarInterval,
ESCalendarIntervalUnit,
ESFixedInterval,
ESFixedIntervalUnit,
} from './utils/chrono/elasticsearch';
6 changes: 3 additions & 3 deletions packages/charts/src/utils/chrono/chrono.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ import {
formatTimeObj,
diffTimeObjs,
} from './moment';
import { CalendarIntervalUnit, CalendarObj, DateTime, FixedIntervalUnit, Minutes, UnixTimestamp } from './types';
import { CalendarIntervalUnit, DateTime, FixedIntervalUnit, Minutes, UnixTimestamp } from './types';

/** @internal */
export function addTime(
dateTime: DateTime,
timeZone: string | undefined,
unit: keyof CalendarObj,
unit: CalendarIntervalUnit | FixedIntervalUnit,
count: number,
): UnixTimestamp {
return timeObjToUnixTimestamp(addTimeToObj(getTimeObj(dateTime, timeZone), unit, count));
Expand All @@ -34,7 +34,7 @@ export function addTime(
export function subtractTime(
dateTime: DateTime,
timeZone: string | undefined,
unit: keyof CalendarObj,
unit: CalendarIntervalUnit | FixedIntervalUnit,
count: number,
): UnixTimestamp {
return timeObjToUnixTimestamp(subtractTimeToObj(getTimeObj(dateTime, timeZone), unit, count));
Expand Down
8 changes: 4 additions & 4 deletions packages/charts/src/utils/chrono/elasticsearch.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('snap to interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'calendar', unit: 'd', quantity: 1 },
{ type: 'calendar', unit: 'd', value: 1 },
'start',
'UTC',
);
Expand All @@ -26,7 +26,7 @@ describe('snap to interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'calendar', unit: 'd', quantity: 1 },
{ type: 'calendar', unit: 'd', value: 1 },
'end',
'UTC',
);
Expand All @@ -37,7 +37,7 @@ describe('snap to interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'fixed', unit: 'm', quantity: 30 },
{ type: 'fixed', unit: 'm', value: 30 },
'start',
'UTC',
);
Expand All @@ -48,7 +48,7 @@ describe('snap to interval', () => {
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z');
const snappedDate = snapDateToESInterval(
initialDate.toMillis(),
{ type: 'fixed', unit: 'm', quantity: 30 },
{ type: 'fixed', unit: 'm', value: 30 },
'end',
'UTC',
);
Expand Down
Loading