-
Notifications
You must be signed in to change notification settings - Fork 121
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 domain to interval #1253
Merged
markov00
merged 13 commits into
elastic:master
from
markov00:2021_07_14-heatmap_snap_time
Jul 30, 2021
Merged
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
75b6597
feat: add snapDateInterval fn
markov00 6a5fcdf
fix: snap time interval in heatmap domain
markov00 c93deff
refactor: fix review comments
markov00 32f33c7
refactor: abstract date time functions in chrono
markov00 3abe17f
Merge branch 'master' into 2021_07_14-heatmap_snap_time
markov00 5903998
test: trying adding VRT
markov00 fd861da
Merge branch '2021_07_14-heatmap_snap_time' of github.com:markov00/el…
markov00 89c15d3
test: fix vrt screenshot
markov00 b5b28ea
fix: cast the domain to the accepted format
markov00 aafb7ea
Merge branch 'master' into 2021_07_14-heatmap_snap_time
markov00 f471fd0
refactor: remove long switch
markov00 4618415
Merge branch '2021_07_14-heatmap_snap_time' of github.com:markov00/el…
markov00 2b0f8ed
Merge branch 'master' into 2021_07_14-heatmap_snap_time
markov00 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* | ||
* 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 { DateTime } from 'luxon'; | ||
|
||
import { snapDateToInterval } from './date_time'; | ||
|
||
describe('snap to interval', () => { | ||
it('should snap to begin of calendar interval', () => { | ||
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z'); | ||
const snappedDate = snapDateToInterval( | ||
initialDate.toMillis(), | ||
{ type: 'calendar', unit: 'd', quantity: 1 }, | ||
'start', | ||
'UTC', | ||
); | ||
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T00:00:00.000Z'); | ||
}); | ||
|
||
it('should snap to end of calendar interval', () => { | ||
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z'); | ||
const snappedDate = snapDateToInterval( | ||
initialDate.toMillis(), | ||
{ type: 'calendar', unit: 'd', quantity: 1 }, | ||
'end', | ||
'UTC', | ||
); | ||
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T23:59:59.999Z'); | ||
}); | ||
|
||
it('should snap to begin of fixed interval', () => { | ||
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z'); | ||
const snappedDate = snapDateToInterval( | ||
initialDate.toMillis(), | ||
{ type: 'fixed', unit: 'm', quantity: 30 }, | ||
'start', | ||
'UTC', | ||
); | ||
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T07:00:00.000Z'); | ||
}); | ||
|
||
it('should snap to end of fixed interval', () => { | ||
const initialDate = DateTime.fromISO('2020-01-03T07:00:01Z'); | ||
const snappedDate = snapDateToInterval( | ||
initialDate.toMillis(), | ||
{ type: 'fixed', unit: 'm', quantity: 30 }, | ||
'end', | ||
'UTC', | ||
); | ||
expect(DateTime.fromMillis(snappedDate, { zone: 'utc' }).toISO()).toBe('2020-01-03T07:29:59.999Z'); | ||
}); | ||
}); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,8 @@ | |
|
||
import moment from 'moment-timezone'; | ||
|
||
import { TimeMs } from '../../common/geometry'; | ||
|
||
/** @internal */ | ||
export function getMomentWithTz(date: number | Date, timeZone?: string) { | ||
if (timeZone === 'local' || !timeZone) { | ||
|
@@ -18,3 +20,98 @@ export function getMomentWithTz(date: number | Date, timeZone?: string) { | |
} | ||
return moment.tz(date, timeZone); | ||
} | ||
|
||
/** @internal */ | ||
export type UnixTimestamp = TimeMs; | ||
|
||
type CalendarIntervalUnit = | ||
| 'minute' | ||
| 'm' | ||
| 'hour' | ||
| 'h' | ||
| 'day' | ||
| 'd' | ||
| 'week' | ||
| 'w' | ||
| 'month' | ||
| 'M' | ||
| 'quarter' | ||
| 'q' | ||
| 'year' | ||
| 'y'; | ||
|
||
type FixedIntervalUnit = 'ms' | 's' | 'm' | 'h' | 'd'; | ||
|
||
const FIXED_UNIT_TO_BASE: Record<FixedIntervalUnit, TimeMs> = { | ||
ms: 1, | ||
s: 1000, | ||
m: 1000 * 60, | ||
h: 1000 * 60 * 60, | ||
d: 1000 * 60 * 60 * 24, | ||
}; | ||
|
||
/** @internal */ | ||
export type CalendarInterval = { | ||
type: 'calendar'; | ||
unit: CalendarIntervalUnit; | ||
quantity: number; | ||
}; | ||
/** @internal */ | ||
export type FixedInterval = { | ||
type: 'fixed'; | ||
unit: FixedIntervalUnit; | ||
quantity: number; | ||
}; | ||
function isCalendarInterval(interval: CalendarInterval | FixedInterval): interval is CalendarInterval { | ||
return interval.type === 'calendar'; | ||
} | ||
|
||
/** @internal */ | ||
export function snapDateToInterval( | ||
date: number | Date, | ||
interval: CalendarInterval | FixedInterval, | ||
snapTo: 'start' | 'end', | ||
timeZone?: string, | ||
): UnixTimestamp { | ||
const momentDate = getMomentWithTz(date, timeZone); | ||
return isCalendarInterval(interval) | ||
? calendarIntervalSnap(momentDate, interval, snapTo).valueOf() | ||
: fixedIntervalSnap(momentDate, interval, snapTo).valueOf(); | ||
} | ||
|
||
function calendarIntervalSnap(date: moment.Moment, interval: CalendarInterval, snapTo: 'start' | 'end') { | ||
const momentUnitName = esCalendarIntervalsToMoment(interval); | ||
return snapTo === 'start' ? date.startOf(momentUnitName) : date.endOf(momentUnitName); | ||
} | ||
function fixedIntervalSnap(date: moment.Moment, interval: FixedInterval, snapTo: 'start' | 'end') { | ||
const unitMultiplier = interval.quantity * FIXED_UNIT_TO_BASE[interval.unit]; | ||
const roundedDate = Math.floor(date.valueOf() / unitMultiplier) * unitMultiplier; | ||
return snapTo === 'start' ? roundedDate : roundedDate + unitMultiplier - 1; | ||
} | ||
|
||
function esCalendarIntervalsToMoment(interval: CalendarInterval): moment.unitOfTime.StartOf { | ||
// eslint-disable-next-line default-case | ||
switch (interval.unit) { | ||
case 'minute': | ||
case 'm': | ||
return 'minutes'; | ||
case 'hour': | ||
case 'h': | ||
return 'hour'; | ||
case 'day': | ||
case 'd': | ||
return 'day'; | ||
case 'week': | ||
case 'w': | ||
return 'week'; | ||
case 'month': | ||
case 'M': | ||
return 'month'; | ||
case 'quarter': | ||
case 'q': | ||
return 'quarter'; | ||
case 'year': | ||
case 'y': | ||
return 'year'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As it's quite a long switch, requiring up to 14 comparisons, it'd be a bit nicer if these were in a map, eg.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in f471fd0 |
||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
/* | ||
* 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 { number } from '@storybook/addon-knobs'; | ||
import { DateTime } from 'luxon'; | ||
import React, { useMemo } from 'react'; | ||
|
||
import { Chart, Heatmap, RecursivePartial, ScaleType, Settings } from '../../packages/charts/src'; | ||
import { Config } from '../../packages/charts/src/chart_types/heatmap/layout/types/config_types'; | ||
import { getRandomNumberGenerator } from '../../packages/charts/src/mocks/utils'; | ||
nickofthyme marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const rng = getRandomNumberGenerator(); | ||
const start = DateTime.fromISO('2021-03-27T20:00:00', { zone: 'CET' }); | ||
const end = DateTime.fromISO('2021-03-28T11:00:00', { zone: 'CET' }); | ||
const data = [...new Array(14)].flatMap((d, i) => { | ||
return [ | ||
[start.plus({ hour: i }).toMillis(), 'cat A', rng(0, 10)], | ||
[start.plus({ hour: i }).toMillis(), 'cat B', rng(0, 10)], | ||
[start.plus({ hour: i }).toMillis(), 'cat C', rng(0, 10)], | ||
[start.plus({ hour: i }).toMillis(), 'cat D', rng(0, 10)], | ||
[start.plus({ hour: i }).toMillis(), 'cat E', rng(0, 10)], | ||
]; | ||
}); | ||
|
||
export const Example = () => { | ||
const config: RecursivePartial<Config> = useMemo( | ||
() => ({ | ||
grid: { | ||
cellHeight: { | ||
min: 20, | ||
}, | ||
stroke: { | ||
width: 0, | ||
color: '#D3DAE6', | ||
}, | ||
}, | ||
cell: { | ||
maxWidth: 'fill', | ||
maxHeight: 3, | ||
label: { | ||
visible: false, | ||
}, | ||
border: { | ||
stroke: 'transparent', | ||
strokeWidth: 0, | ||
}, | ||
}, | ||
yAxisLabel: { | ||
visible: true, | ||
width: 'auto', | ||
padding: { left: 10, right: 10 }, | ||
}, | ||
xAxisLabel: { | ||
formatter: (value: string | number) => { | ||
return DateTime.fromMillis(value as number).toFormat('HH:mm:ss', { timeZone: 'UTC' }); | ||
}, | ||
}, | ||
}), | ||
[], | ||
); | ||
|
||
const startTimeOffset = number('start time offset', 0, { | ||
min: -1000 * 60 * 60 * 24, | ||
max: 1000 * 60 * 60 * 10, | ||
step: 1000, | ||
range: true, | ||
}); | ||
const endTimeOffset = number('end time offset', 0, { | ||
min: -1000 * 60 * 60 * 10, | ||
max: 1000 * 60 * 60 * 24, | ||
step: 1000, | ||
range: true, | ||
}); | ||
|
||
return ( | ||
<> | ||
<div style={{ fontFamily: 'monospace', fontSize: 10, paddingBottom: 5 }}> | ||
{DateTime.fromMillis(start.toMillis() + startTimeOffset).toISO()} to{' '} | ||
{DateTime.fromMillis(end.toMillis() + endTimeOffset).toISO()} | ||
</div> | ||
<Chart className="story-chart"> | ||
<Settings | ||
xDomain={{ | ||
min: start.toMillis() + startTimeOffset, | ||
max: end.toMillis() + endTimeOffset, | ||
minInterval: 1000 * 60 * 60, | ||
}} | ||
/> | ||
<Heatmap | ||
id="heatmap1" | ||
colorScale={ScaleType.Linear} | ||
colors={['white', 'blue']} | ||
data={data} | ||
xAccessor={(d) => d[0]} | ||
yAccessor={(d) => d[1]} | ||
valueAccessor={(d) => d[2]} | ||
valueFormatter={(d) => d.toFixed(2)} | ||
ySortPredicate="numAsc" | ||
xScaleType={ScaleType.Time} | ||
config={config} | ||
/> | ||
</Chart> | ||
</> | ||
); | ||
}; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type of the 1st and 2nd params of
bisectLeft
need to match, and we're typeguarding the 2nd param tonumber
, so it's slightly tighter to sayas number[]
. Though it's not truthful, maybe we could brainstorm so I can learn about whetherOrdinalDomain
needs to be(number | string)[]
and can't benumber[] | string[]
eg. by coercing numbers to strings if at least one element is a string. Though there's still the lack of type coherence betweenxValues
andstartValue
- probably they correlate strongly (otherwise we can't doas any
oras number[]
here) but this seems to be lost on TSThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the type of
bisectLeft
is incorrect or too restrictive (we can't overrule it as it's beyond what the D3 TS API guarantees, but iirc these are 3rd party post hoc libs, not canonical Bostock contract) then maybe it'd be better to factor it out into ourbisectLeft
utility with the desired types, so we can easily change from D3 if needed. I think we could already use the also logarithmically bisectingmonotonicHillClimb
instead ofd3.bisectLeft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is one main reason for having
(number | string)[]
asOrdinalDomain
: we like to preserve the original type, coming from the user data, to return the original values on the interaction callbacks (brushing or clicking)We can probably force it to be
number[] | string[]
but I don't have strong opinion hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more info may strengthen the case for
string[] | number[]
:D3
bisectLeft
, and generally, logarithmic search requires total order, which we likely ensure by sorting the values beforehand(*).Out of the properties of total order, transitivity is most important: if a <= b and b <= c then a <= c must be true (it's not necessarily true for partial orders, eg. when a number is defined non-comparable to a string).
(*) The mechanism doesn't matter, eg.
[].sort
also relies on a predicate plus array which together guarantee a total order, otherwise the order is ill defined: EcmaScript requires transitivity for predictable results:If a <CF b and b <CF c, then a <CF c (transitivity of <CF)
.Sorting
(number | string)[]
with a<
predicate doesn't lead to total order. Example:So a sensible option is to string-compare all elements if at least one element in the array is not a number, which is the default
sort
predicate (and number-compare for all-numeric arrays, which is not the defaultsort
predicate). Instead of checking for this at the place of sorting, we may as well come clean and convert upfront. As you suggested Marco, the original value can still be retained. The effect of the conversion though is that a user-supplied[5, '5']
will map to['5', '5']
so an equally good method is, documenting in the API, perhaps even enforcing via TS, that the domain values be homogeneous.There can of course be other sorting rules, eg. all strings are moved to the end (tiebreaker rule for when a string and a number are compared), and within both the strings and numbers, there's full order