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(partition): getLegendItemsExtra no longer assumes a singleton #1199

Merged
merged 7 commits into from
Jun 10, 2021
Merged

fix(partition): getLegendItemsExtra no longer assumes a singleton #1199

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bfbcd08
fix: getLegendItemsExtra no longer assumes a singleton
monfera Jun 8, 2021
46f13d6
fix: icicle and flame should ignore the grouping
monfera Jun 9, 2021
ef40b2d
Merge remote-tracking branch 'origin/master' into singleton-partition…
monfera Jun 9, 2021
874b94a
chore: remove disused function
monfera Jun 10, 2021
da812fa
refactor: exported `HIERARCHY_ROOT_KEY` and also added `NULL_SMALL_MU…
monfera Jun 10, 2021
afc546b
chore: api update and api generator path update
monfera Jun 10, 2021
d7a18fd
Merge remote-tracking branch 'origin/master' into singleton-partition…
monfera Jun 10, 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
16 changes: 14 additions & 2 deletions packages/charts/src/chart_types/partition_chart/layout/utils/group_by_rollup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export interface NodeDescriptor {
export type ArrayEntry = [Key, ArrayNode];
/** @public */
export type HierarchyOfArrays = Array<ArrayEntry>;

/** @public */
export interface ArrayNode extends NodeDescriptor {
[CHILDREN_KEY]: HierarchyOfArrays;
Expand All @@ -65,6 +66,7 @@ export interface ArrayNode extends NodeDescriptor {
}

type HierarchyOfMaps = Map<Key, MapNode>;

interface MapNode extends NodeDescriptor {
[CHILDREN_KEY]?: HierarchyOfMaps;
[PARENT_KEY]?: ArrayNode;
Expand All @@ -90,26 +92,32 @@ export type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;
export const entryKey = ([key]: ArrayEntry) => key;
/** @public */
export const entryValue = ([, value]: ArrayEntry) => value;

/** @public */
export function depthAccessor(n: ArrayEntry) {
return entryValue(n)[DEPTH_KEY];
}

/** @public */
export function aggregateAccessor(n: ArrayEntry): number {
return entryValue(n)[AGGREGATE_KEY];
}

/** @public */
export function parentAccessor(n: ArrayEntry): ArrayNode {
return entryValue(n)[PARENT_KEY];
}

/** @public */
export function childrenAccessor(n: ArrayEntry) {
return entryValue(n)[CHILDREN_KEY];
}

/** @public */
export function sortIndexAccessor(n: ArrayEntry) {
return entryValue(n)[SORT_INDEX_KEY];
}

/** @public */
export function pathAccessor(n: ArrayEntry) {
return entryValue(n)[PATH_KEY];
Expand Down Expand Up @@ -185,7 +193,11 @@ function getRootArrayNode(): ArrayNode {
}

/** @internal */
export function mapsToArrays(root: HierarchyOfMaps, sortSpecs: (NodeSorter | null)[]): HierarchyOfArrays {
export function mapsToArrays(
root: HierarchyOfMaps,
sortSpecs: (NodeSorter | null)[],
innerGroups: LegendPath,
): HierarchyOfArrays {
const groupByMap = (node: HierarchyOfMaps, parent: ArrayNode) => {
const items = Array.from(
node,
Expand Down Expand Up @@ -230,7 +242,7 @@ export function mapsToArrays(root: HierarchyOfMaps, sortSpecs: (NodeSorter | nul
mapNode[PATH_KEY] = newPath; // in-place mutation, so disabled `no-param-reassign`
mapNode.children.forEach((entry) => buildPaths(entry, newPath));
};
buildPaths(tree[0], []);
buildPaths(tree[0], innerGroups);
return tree;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ const groupByRollupAccessors = [() => null, (d: any) => d.sitc1];

describe('Test', () => {
test('getHierarchyOfArrays should omit zero and negative values', () => {
const outerResult = getHierarchyOfArrays(rawFacts, valueAccessor, groupByRollupAccessors, []);
const outerResult = getHierarchyOfArrays(rawFacts, valueAccessor, groupByRollupAccessors, [], []);
expect(outerResult.length).toBe(1);

const results = outerResult[0];
Expand Down
6 changes: 5 additions & 1 deletion packages/charts/src/chart_types/partition_chart/layout/viewmodel/hierarchy_of_arrays.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import { LegendItemExtraValues } from '../../../../common/legend';
import { SeriesKey } from '../../../../common/series_id';
import { Relation } from '../../../../common/text_utils';
import { LegendPath } from '../../../../state/actions/legend';
import { IndexedAccessorFn } from '../../../../utils/accessor';
import { Datum, ValueAccessor, ValueFormatter } from '../../../../utils/common';
import { Layer } from '../../specs';
Expand Down Expand Up @@ -60,6 +61,7 @@ export function getHierarchyOfArrays(
valueAccessor: ValueAccessor,
groupByRollupAccessors: IndexedAccessorFn[],
sortSpecs: (NodeSorter | null)[],
innerGroups: LegendPath,
): HierarchyOfArrays {
const aggregator = aggregators.sum;

Expand All @@ -76,7 +78,7 @@ export function getHierarchyOfArrays(
// We can precompute things invariant of how the rectangle is divvied up.
// By introducing `scale`, we no longer need to deal with the dichotomy of
// size as data value vs size as number of pixels in the rectangle
return mapsToArrays(groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts), sortSpecs);
return mapsToArrays(groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts), sortSpecs, innerGroups);
}

const sorter = (layout: PartitionLayout) => ({ sortPredicate }: Layer, i: number) =>
Expand All @@ -96,13 +98,15 @@ export function partitionTree(
layers: Layer[],
defaultLayout: PartitionLayout,
partitionLayout: PartitionLayout = defaultLayout,
innerGroups: LegendPath,
) {
return getHierarchyOfArrays(
data,
valueAccessor,
// eslint-disable-next-line no-shadow
[() => HIERARCHY_ROOT_KEY, ...layers.map(({ groupByRollup }) => groupByRollup)],
[null, ...layers.map(sorter(partitionLayout))],
innerGroups,
);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/charts/src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ const rawChildNodes = (
const icicleLayout = isIcicle(partitionLayout);
const icicleValueToAreaScale = width / totalValue;
const icicleAreaAccessor = (e: ArrayEntry) => icicleValueToAreaScale * mapEntryValue(e);
const icicleRowHeight = height / maxDepth;
const icicleRowHeight = height / (maxDepth - 1);
const result = sunburst(tree, icicleAreaAccessor, { x0: 0, y0: -icicleRowHeight }, true, false, icicleRowHeight);
return icicleLayout
? result
Expand Down
13 changes: 13 additions & 0 deletions packages/charts/src/chart_types/partition_chart/partition.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
nickofthyme marked this conversation as resolved.
Show resolved Hide resolved
{ index: 0, value: 'A' },
],
Expand All @@ -142,6 +143,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 0, value: 'A' },
{ index: 0, value: 'A' },
Expand All @@ -155,6 +157,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 0, value: 'A' },
{ index: 1, value: 'B' },
Expand All @@ -168,6 +171,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 1, value: 'B' },
],
Expand All @@ -180,6 +184,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 1, value: 'B' },
{ index: 0, value: 'A' },
Expand All @@ -193,6 +198,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 1, value: 'B' },
{ index: 1, value: 'B' },
Expand All @@ -206,6 +212,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'C',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 2, value: 'C' },
],
Expand All @@ -218,6 +225,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 2, value: 'C' },
{ index: 0, value: 'A' },
Expand All @@ -231,6 +239,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{ index: 0, value: HIERARCHY_ROOT_KEY },
{ index: 2, value: 'C' },
{ index: 1, value: 'B' },
Expand All @@ -256,6 +265,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{
index: 0,
value: HIERARCHY_ROOT_KEY,
Expand All @@ -274,6 +284,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'A',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{
index: 0,
value: HIERARCHY_ROOT_KEY,
Expand Down Expand Up @@ -309,6 +320,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'C',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{
index: 0,
value: HIERARCHY_ROOT_KEY,
Expand All @@ -327,6 +339,7 @@ describe('Retain hierarchy even with arbitrary names', () => {
childId: 'B',
color: 'rgba(128, 0, 0, 0.5)',
path: [
{ index: 0, value: '' },
{
index: 0,
value: HIERARCHY_ROOT_KEY,
Expand Down
12 changes: 0 additions & 12 deletions packages/charts/src/chart_types/partition_chart/state/selectors/geometries.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,15 +233,3 @@ export const partitionDrilldownFocus = createCachedSelector(
return { currentFocusX0, currentFocusX1, prevFocusX0, prevFocusX1, smAccessorValue, index, innerIndex };
}),
)((state) => state.chartId);

/** @internal */
export const partitionGeometries = createCachedSelector(
[partitionMultiGeometries],
(multiGeometries: ShapeViewModel[]) => {
return [
multiGeometries.length > 0 // singleton!
? multiGeometries[0]
: nullShapeViewModel(),
];
},
)(getChartIdSelector);
34 changes: 17 additions & 17 deletions ...ges/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,25 +69,25 @@ describe('Partition - Legend item extra values', () => {

const extraValues = getLegendItemsExtra(store.getState());
expect([...extraValues.keys()]).toEqual([
'0',
'0__0',
'0__0__0',
'0__0__0__0',
'0__0__0__0__0',
'0__0__0__0__1',
'0__0__0__1',
'0__0__0__1__0',
'0__0__0__1__1',
'0__0__0__1__2',
'0__0__1',
'0__0__1__0',
'0__0__1__0__0',
'0__0__1__0__1',
'0__0__1__1',
'0__0__1__1__0',
'0__0__1__1__1',
'0__0__1__2',
'0__1',
'0__1__0',
'0__1__0__0',
'0__1__0__1',
'0__1__1',
'0__1__1__0',
'0__1__1__1',
'0__1__2',
'0__1__2__0',
'0__1__2__1',
'0__0__1__2__0',
'0__0__1__2__1',
]);
expect(extraValues.values()).toMatchSnapshot();
});
Expand All @@ -97,7 +97,7 @@ describe('Partition - Legend item extra values', () => {
MockStore.addSpecs([settings, spec], store);

const extraValues = getLegendItemsExtra(store.getState());
expect([...extraValues.keys()]).toEqual(['0', '0__0', '0__1']);
expect([...extraValues.keys()]).toEqual(['0__0', '0__0__0', '0__0__1']);
Copy link
Member

Choose a reason for hiding this comment

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

["😐","😐😐","😐"]

Copy link
Contributor Author

@monfera monfera Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

the API is not changed itself, but the actual value now includes one more depth level

I agree, I haven't considered it as the type signature hasn't changed.

Are we sure we want to add an unclear {index: 0, value: ""} at level 0 when we are not using small multiples?

Looks possible to avoid this. I very briefly thought about it while changing the code and brushed it aside as it also creates a special case that the user code needs to handle with conditional branching in their event handler depending on whether or not it's a small multiples ensemble. When there's no clear benefit either way, I'm leaning toward avoiding corner cases (of eg. singleton chart) as it complicates code paths, and sometimes forces types into union types in our code (not in this case) or in the userland, which, all things being equal, is good to avoid.

Nonetheless I'm open to conditionally adding the first layer if you have an argument for it

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you about not having corner cases.
I'm wondering if useful to have a more descriptive value there, a value: null o value:__no_sm__ can help understand better what that path value is actually about? if not we can probably document the LegendPath type describing that the first 2 elements are always [sm category, an empty root node, .....]

expect(extraValues.values()).toMatchSnapshot();
});

Expand All @@ -107,14 +107,14 @@ describe('Partition - Legend item extra values', () => {

const extraValues = getLegendItemsExtra(store.getState());
expect([...extraValues.keys()]).toEqual([
'0',
'0__0',
'0__0__0',
'0__0__0__0',
'0__0__0__1',
'0__0__1',
'0__1',
'0__1__0',
'0__1__1',
'0__1__2',
'0__0__1__0',
'0__0__1__1',
'0__0__1__2',
]);
expect(extraValues.values()).toMatchSnapshot();
});
Expand Down
11 changes: 9 additions & 2 deletions packages/charts/src/chart_types/partition_chart/state/selectors/get_legend_items_extra.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,15 @@ import { getTrees } from './tree';
export const getLegendItemsExtra = createCachedSelector(
[getPartitionSpec, getSettingsSpecSelector, getTrees],
(spec, { legendMaxDepth }, trees): Map<SeriesKey, LegendItemExtraValues> => {
const emptyMap = new Map<SeriesKey, LegendItemExtraValues>();
return spec && !Number.isNaN(legendMaxDepth) && legendMaxDepth > 0
? getExtraValueMap(spec.layers, spec.valueFormatter, trees[0].tree, legendMaxDepth) // singleton! wrt inner small multiples
: new Map<SeriesKey, LegendItemExtraValues>();
? trees.reduce((result, { tree }) => {
const treeData = getExtraValueMap(spec.layers, spec.valueFormatter, tree, legendMaxDepth);
for (const [key, value] of treeData) {
result.set(key, value);
}
return result;
}, emptyMap)
: emptyMap;
},
)(getChartIdSelector);
Loading