feat(xy): adaptive tick raster #1420

monfera · 2021-10-07T18:59:50Z

Summary

PR for lowering the targeted tick count until a compliant tick raster is the result. Compliance criteria are informed by the need to convey truth on the chart, and also by dataviz best practices:

All tick labels must fit (none should overlap with any another);
The tick labels should have a preference for a (generally) decimal base, eg. 0-5-10 over 0-4-8-12
Nicing, if applies, should be observed
Landmark ticks, such as zero, or the extreme tick labels in case of nicing, must be preserved
There must be at least two distinct ticks, otherwise the screen projection can't be correlated with the shown data, rendering the chart meaningless
All ticks must have a unique label, otherwise there's ambiguity about which represents the true value (if any)
The raster should yield even spacing of the tick/gridlines even if screenspace tick label sizes vary
Coarser semantic units (eg. hours) should be split into an integral number of smaller units (eg. 1,2,3,4,5,6,10,12,15,20,30mins) - note that due to other constraints, some will be not generated, eg. a cadence of 3 minutes (not just our constraints but also what matured into underlying scale generators like D3, which we can relax if needed in the future)

Details

The current method relies on tick label culling (and optionally, tick/gridline culling) for avoiding overlap and duplicated tick labels, but the (inherently post hoc) culling of the tick labels doesn't guarantee compliance with the above items. For examples:

maybe, due to rounding in the formatter, some labels are culled due to duplication, but the surviving one isn't necessarily the one at the actual position of the shown value
there could be multiple generated ticks, but culling can remove some such that less than two remain
unusual cadences may be generated, eg. an unusual 3-minute cadence; decimal boundaries can't be prioritized by culling
variable tick label length (in pixels) can yield an uneven raster, where the distance between any two adjacent ticks can significantly vary
the zero value, and/or the last tick label, placed expressly by nicing, may be culled

In short, we deactivate the culling of a single raster; instead, we iteratively generate the raster that's closest to the desired tick count, while complying with the rules

adaptiveTickCount is set to false, ie. old mode, to make all but one VRT images identical with the current snapshots

Issues

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
The proper feature labels have been added (e.g. :interactions, :axis)
The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
All related issues have been linked (i.e. closes #123, fixes #123)
New public API exports have been added to packages/charts/src/index.ts
Unit tests have been added or updated to match the most common scenarios
The proper documentation and/or storybook story has been added or updated
The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

…ld be better

…ess in its timezone

…be a bit better

… be labeled

…allowing decreasing order

monfera · 2021-10-10T11:58:37Z

.eslintrc.js

@@ -161,7 +161,7 @@ module.exports = {
      },
    ],
    'sort-keys': 0,
-    'no-irregular-whitespace': 'error',
+    'no-irregular-whitespace': 'warn',


It's fair play to use non-breaking space or whatever in strings. If such a value is commented out, it triggers this condition, and it can't be locally bypassed with an ignore line

monfera · 2021-10-10T12:00:24Z

packages/charts/src/chart_types/xy_chart/axes/axes_sizes.ts

+};
+
+/** @internal */
+export function getAxesDimensions(


The former getAxesDimensions got split into a reducer part (this here) and an actual calculation part (getAxisSizeForLabel)

monfera · 2021-10-10T12:02:04Z

packages/charts/src/chart_types/xy_chart/axes/axes_sizes.ts

+  return axisLayerCount * maxLabelBoxGirth;
+};
+
+const getAxisSizeForLabel = (


The logic in getAxisSizeForLabel matches the former calc in getAxesDimensions except for the use of getAllAxisLayersGirth which permits an axis girth reflecting multiple axis tick layers

monfera · 2021-10-10T12:02:40Z

packages/charts/src/chart_types/xy_chart/renderer/canvas/panels/title.ts

-  const maxLabelBoxSize = horizontal ? maxLabelBboxHeight : maxLabelBboxWidth;
-  const labelSize = tickLabel.visible ? maxLabelBoxSize + innerPad(tickLabel.padding) + outerPad(tickLabel.padding) : 0;
+  const maxLabelBoxGirth = horizontal ? maxLabelBboxHeight : maxLabelBboxWidth;
+  const allLayersGirth = getAllAxisLayersGirth(tickLabel, maxLabelBoxGirth, horizontal);


Drying up logic

monfera · 2021-10-10T12:05:05Z

packages/charts/src/chart_types/xy_chart/state/selectors/compute_axis_ticks_dimensions.ts

@@ -91,30 +93,40 @@ const getJoinedVisibleAxesData = createCustomCachedSelector(
    }, new Map()),
 );

+/** @internal */
+export const getLabelBox = (


Breaking out this from computeAxisTicksDimensionsSelector in part to detach the logic from the reduction, and in part to enable reuse of the logic

monfera · 2021-10-10T13:03:44Z

packages/charts/src/chart_types/xy_chart/state/selectors/get_specs.ts

+export const axisSpecsLookupSelector = createCustomCachedSelector(
+  getAxisSpecsSelector,
+  (axisSpecs: AxisSpec[]): Map<string, AxisSpec> => axisSpecs.reduce((acc, spec) => acc.set(spec.id, spec), new Map()),
+);


Using it is O(1)

monfera · 2021-10-10T14:13:11Z

test this

markov00

Code changes LGTM
Tested locally

The changes on the number of ticks in a log scale are done on purpose to improve the scale reading.

There are few TODOs to fix on a subsequent PR:

there is a visible reduction of the number of ticks rendered for "standard" time axis, this happens due to the changes in the cadence on the time intervals (we don't allow 2 min steps for example, but we allow only 1 minute or 5 minute steps). This, if needed can be fixed in a subsequent PR
fix the generation of too many ticks under specific conditions (see the high volume story) (see VRT related changes)
remove the double-time axis example from storybook (one VRT to remove)
fix the deduplication under specific conditions: with non-time axis we should keep all the generated ticks and avoid the filtering in enableDuplicatedTicks to make the ticks reduction works correctly. (a VRT with wrong deduplication rendering was added and that need to be fixed ff686d7)

nickofthyme

Code change LGTM, tested logic in storybook and looks to match defined conditions.

The time ticks are definitely more sparse than previously but I think this is cover in the future work.

I appreciate the added ticks for missing labels, I'd like to see this go future where we can define primary and secondary ticks and labels say 1000s on primary and 100 on secondary. I see similarities between this idea and the timeslip multilayer axis.

nickofthyme · 2021-10-12T17:11:24Z

storybook/stories/area/22_with_time_timeslip_2.story.tsx

+  /*
+    const midAxisLabelFormatter = (d: any) =>
+      `${whiskers ? ' ' : ''}${new Intl.DateTimeFormat('en-US', { hour: 'numeric' }).format(d).padStart(2, '0')}  `;
+    const bottomAxisLabelFormatter = (d: any) =>
+      `${whiskers ? ' ' : ''}${new Intl.DateTimeFormat('en-US', {
+        year: 'numeric',
+        month: 'long',
+        day: 'numeric',
+      }).format(d)}  `;
+  */


Can we avoid committing comments? It's just the same to add a boolean knob here that uses this formatter or another. Commenting it serves no purpose.

I agree, though as it's just a story, it might be fine for a short time, the upcoming PR removes this file

# [38.0.0](v37.0.0...v38.0.0) (2021-10-15) ### Bug Fixes * **deps:** update dependency @elastic/eui to v39 ([#1422](#1422)) ([2ee97aa](2ee97aa)) * **goal:** reduce whitespace for circular charts ([#1413](#1413)) ([6517523](6517523)) * **interactions:** change allowBrushingLastHistogramBin to true ([#1396](#1396)) ([9fa9783](9fa9783)) * **xy:** remove wrongly represented null/missing values in tooltip ([#1415](#1415)) ([e5963a3](e5963a3)), closes [#1414](#1414) ### Code Refactoring * scales ([#1410](#1410)) ([a53a2ba](a53a2ba)) ### Features * **scales:** add `LinearBinary` scale type ([#1389](#1389)) ([9f2e427](9f2e427)) * **xy:** adaptive tick raster ([#1420](#1420)) ([200577b](200577b)) * **xy:** apply the data value formatter to data values over bars ([#1419](#1419)) ([e673fc7](e673fc7)) ### BREAKING CHANGES * **interactions:** allowBrushingLastHistogramBucket renamed to allowBrushingLastHistogramBin on the Settings component defaults true and is only applied for histogram type charts * LogScaleOptions.logBase` is now a `number` instead of the object enum `LogBase`. Some edge case data or configuration _might_, with a deemed low likelihood, lead to a situation where the earlier version would have silently not rendered a bar, line or point, while the new code doesn't `catch`, therefore throw an exception (see the last item). General risk of regressions due to the quantity of code changes (altogether 3.5k)

nickofthyme · 2021-10-15T14:01:04Z

🎉 This PR is included in version 38.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

monfera added 4 commits October 7, 2021 19:13

feat: tick raster

2434d8f

chore: unify

1a6a242

refactor: dry up resulting in getLabelBox

c2557bb

feat: require at least two ticks

3dee3a6

monfera added enhancement New feature or request wip work in progress :axis Axis related issue :data Data/series/scales related issue :xy Bar/Line/Area chart related labels Oct 7, 2021

monfera marked this pull request as draft October 7, 2021 19:00

feat: require unique ticks

09121f9

monfera force-pushed the duplicated-tick-label-fix branch from 58e548f to 09121f9 Compare October 7, 2021 19:09

monfera added 2 commits October 8, 2021 09:51

feat: bail with equal tick count

0d8de3b

perf: avoid measurement if tick count remained the same

b0bab66

monfera force-pushed the duplicated-tick-label-fix branch from 33835fe to b0bab66 Compare October 8, 2021 08:05

monfera added 15 commits October 8, 2021 11:31

chore: inconsequential reorder

f7960e0

test: admit pic - the log tick for 9 is preferred

53370bd

test: admit pic - more regular tick placement

24eb995

test: admit pic - y is based on zero and pitch is decimal friendly five

433fd32

test: admit pic - decimal friendly pitch of 5mins but maybe 2mins wou…

204cde3

…ld be better

test: admit pic - same 6hr pitch but shifted 3hrs maybe due to roundn…

5089039

…ess in its timezone

test: admit pic - 15min instead of 10min pic though the latter would …

4384fa1

…be a bit better

test: admit pic - ticks for 7 and 9 are useful but maybe 8 should not…

d6c4553

… be labeled

test: admit pic - I didn't bother to check this

4a93585

test: admit pic - nice 2 year pitch

219bb6e

test: admit pic - nice 2 month pitch

72e569e

test: admit pic - daily ticks are also ok

2e14375

test: admit pic - more decimal oriented raster

7158cdb

test: admit pic - zero based and more decimal oriented raster

47f8b51

test: admit pic - addition of unlabeled ticks are also ok

93d390f

monfera added 4 commits October 9, 2021 23:51

fix: avoid numerical precision uncertainties and div zero while also …

6a51a66

…allowing decreasing order

chore: file rename

d7e6901

chore: scale is assured in getMeasuredTicks

b46d8ad

test: type is Projection

d24366b

monfera force-pushed the duplicated-tick-label-fix branch 2 times, most recently from 222d8b9 to fbe20ba Compare October 10, 2021 10:07

test: textAlignment is always there

b85648f

monfera force-pushed the duplicated-tick-label-fix branch from fbe20ba to b85648f Compare October 10, 2021 10:35

test: tickFormatOptions is always there

4af0a38

monfera commented Oct 10, 2021

View reviewed changes

monfera force-pushed the duplicated-tick-label-fix branch 2 times, most recently from 106c0d8 to 4af0a38 Compare October 10, 2021 13:25

add VR test for uneven spaced ticks with deduplication

ff686d7

monfera marked this pull request as ready for review October 12, 2021 09:39

markov00 approved these changes Oct 12, 2021

View reviewed changes

nickofthyme removed the wip work in progress label Oct 12, 2021

nickofthyme approved these changes Oct 12, 2021

View reviewed changes

markov00 merged commit 200577b into elastic:master Oct 13, 2021

monfera mentioned this pull request Oct 14, 2021

chore: import circularity #1425

Merged

3 tasks

nickofthyme added the released Issue released publicly label Oct 15, 2021

nickofthyme linked an issue Oct 20, 2021 that may be closed by this pull request

Improve axis ticks deduplication #1417

Closed

2 tasks

nickofthyme mentioned this pull request Oct 20, 2021

update elastic-charts to 38.0.1 elastic/kibana#115623

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(xy): adaptive tick raster #1420

feat(xy): adaptive tick raster #1420

monfera commented Oct 7, 2021 •

edited

Loading

monfera Oct 10, 2021

monfera Oct 10, 2021

monfera Oct 10, 2021

monfera Oct 10, 2021

monfera Oct 10, 2021

monfera Oct 10, 2021 •

edited

Loading

monfera commented Oct 10, 2021

markov00 left a comment •

edited

Loading

nickofthyme left a comment

nickofthyme Oct 12, 2021

monfera Oct 12, 2021 •

edited

Loading

nickofthyme commented Oct 15, 2021

feat(xy): adaptive tick raster #1420

feat(xy): adaptive tick raster #1420

Conversation

monfera commented Oct 7, 2021 • edited Loading

Summary

Details

Issues

Checklist

monfera Oct 10, 2021

Choose a reason for hiding this comment

monfera Oct 10, 2021

Choose a reason for hiding this comment

monfera Oct 10, 2021

Choose a reason for hiding this comment

monfera Oct 10, 2021

Choose a reason for hiding this comment

monfera Oct 10, 2021

Choose a reason for hiding this comment

monfera Oct 10, 2021 • edited Loading

Choose a reason for hiding this comment

monfera commented Oct 10, 2021

markov00 left a comment • edited Loading

Choose a reason for hiding this comment

nickofthyme left a comment

Choose a reason for hiding this comment

nickofthyme Oct 12, 2021

Choose a reason for hiding this comment

monfera Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

nickofthyme commented Oct 15, 2021

monfera commented Oct 7, 2021 •

edited

Loading

monfera Oct 10, 2021 •

edited

Loading

markov00 left a comment •

edited

Loading

monfera Oct 12, 2021 •

edited

Loading