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

feat(partition): Flame and icicle chart #965

Merged
merged 32 commits into from
Jan 13, 2021
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
59eab63
chore: simplify bail conditioin
monfera Jan 6, 2021
cfb07d7
chore: no longer shadow variables in treemap
monfera Jan 6, 2021
c961de2
chore: solve remaining warnings
monfera Jan 6, 2021
4ebfc69
chore: narrowing the childrenMap type
monfera Jan 6, 2021
fc839b2
chore: eliminate sunburst lint warnings too
monfera Jan 6, 2021
f0107ed
chore: name the closure
monfera Jan 6, 2021
81132d3
refactor: extract out the logic specific for sunburst vs treemap layo…
monfera Jan 6, 2021
329d618
feat: icicle chart
monfera Jan 6, 2021
5ec7718
test: image baseline with no flatMap and apparent vertical truncation
monfera Jan 6, 2021
4e39c41
chore: inconsequential api file change ie just the order
monfera Jan 6, 2021
39e55f8
Merge branch 'master' into icicle-flame
monfera Jan 6, 2021
3f17149
fix: typo
monfera Jan 7, 2021
93dcf25
fix: try to placate ci though no idea why it should not be internal
monfera Jan 7, 2021
3985768
Merge remote-tracking branch 'monfera/icicle-flame' into icicle-flame
monfera Jan 7, 2021
5774b8d
fix: api md update after having to rebuild
monfera Jan 7, 2021
4df838b
fix: topGroove of 0 is not the same as null
monfera Jan 7, 2021
6692738
chore: lint update
monfera Jan 11, 2021
934fa7b
Merge remote-tracking branch 'origin/master' into icicle-flame
monfera Jan 11, 2021
04e1663
chore: lint update 2
monfera Jan 11, 2021
7c1ad36
fix: use full height
monfera Jan 12, 2021
b33bb86
feat: flame chart
monfera Jan 12, 2021
15dbefd
test: image updates
monfera Jan 12, 2021
cc9edc2
test: update stories slash icicle slash 02_unix_flame
monfera Jan 13, 2021
eefbbd7
Merge branch 'master' into icicle-flame
monfera Jan 13, 2021
d6a3893
test: missing flatmmap workaround simplifications by nick
monfera Jan 13, 2021
0aa4781
Update src/chart_types/partition_chart/layout/viewmodel/viewmodel.ts
monfera Jan 13, 2021
ea378fb
refactor: moved out flame utilities
monfera Jan 13, 2021
49f2813
test: shrink width instead of growing height while having a good aspe…
monfera Jan 13, 2021
1fa0580
refactor: regularize access to partititonLayout type and simplify som…
monfera Jan 13, 2021
f9e51f9
test: minimize the impact of assertion to a single property, which is…
monfera Jan 13, 2021
a57b697
test: mock update
monfera Jan 13, 2021
5e9559b
Merge branch 'master' into icicle-flame
monfera Jan 13, 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
5 changes: 3 additions & 2 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ module.exports = {
'global-require': 1,
'import/no-dynamic-require': 1,
'no-shadow': 1,
'no-param-reassign': 1,
'no-param-reassign': [1, { props: false }],
'@typescript-eslint/comma-spacing': 0,
'react/no-array-index-key': 1,
'react/prefer-stateless-function': 1,
'react/require-default-props': 'off',
Expand Down Expand Up @@ -342,7 +343,7 @@ module.exports = {
'prefer-destructuring': [
'warn',
{
array: true,
array: false,
object: true,
},
{
Expand Down
33 changes: 2 additions & 31 deletions .playground/playground.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,39 +38,10 @@

import React from 'react';

import { Chart, Settings, Partition, PartitionLayout } from '../src';
import { Example } from '../stories/icicle/01_unix_icicle';

export class Playground extends React.Component {
render() {
return (
<div className="chart">
<Chart className="story-chart">
<Settings showLegend flatLegend={false} />
<Partition
id="spec_1"
data={[
{ cat1: 'A', cat2: 'A', val: 1 },
{ cat1: 'A', cat2: 'B', val: 1 },
{ cat1: 'B', cat2: 'A', val: 1 },
{ cat1: 'B', cat2: 'B', val: 1 },
{ cat1: 'C', cat2: 'A', val: 1 },
{ cat1: 'C', cat2: 'B', val: 1 },
]}
valueAccessor={(d: any) => d.val as number}
layers={[
{
groupByRollup: (d: any) => d.cat1,
},
{
groupByRollup: (d: any) => d.cat2,
},
]}
config={{
partitionLayout: PartitionLayout.sunburst,
}}
/>
</Chart>
</div>
);
return <Example />;
}
}
6 changes: 4 additions & 2 deletions api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,8 @@ export interface PartitionLayer {
export const PartitionLayout: Readonly<{
sunburst: "sunburst";
treemap: "treemap";
icicle: "icicle";
flame: "flame";
}>;

// @public (undocumented)
Expand Down Expand Up @@ -1958,8 +1960,8 @@ export type YDomainRange = YDomainBase & DomainRange;
// src/chart_types/heatmap/layout/types/config_types.ts:28: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:60: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:61: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:126: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:127:5 - (ae-forgotten-export) The symbol "AnimKeyframe" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/layout/types/config_types.ts:128: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:129:5 - (ae-forgotten-export) The symbol "AnimKeyframe" needs to be exported by the entry point index.d.ts
// src/chart_types/partition_chart/specs/index.ts:48:13 - (ae-forgotten-export) The symbol "NodeColorAccessor" needs to be exported by the entry point index.d.ts
// src/commons/series_id.ts:39:3 - (ae-forgotten-export) The symbol "SeriesKey" needs to be exported by the entry point index.d.ts

Expand Down
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.
2 changes: 2 additions & 0 deletions src/chart_types/partition_chart/layout/types/config_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import { Font, FontFamily, PartialFont } from './types';
export const PartitionLayout = Object.freeze({
sunburst: 'sunburst' as const,
treemap: 'treemap' as const,
icicle: 'icicle' as const,
flame: 'flame' as const,
monfera marked this conversation as resolved.
Show resolved Hide resolved
});

/** @public */
Expand Down
30 changes: 17 additions & 13 deletions src/chart_types/partition_chart/layout/utils/group_by_rollup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,9 @@ interface MapNode extends NodeDescriptor {

export type PrimitiveValue = string | number | null; // there could be more but sufficient for now
type Key = PrimitiveValue;
type Sorter = (a: number, b: number) => number;

export type Sorter = (a: number, b: number) => number;

type NodeSorter = (a: ArrayEntry, b: ArrayEntry) => number;

export const entryKey = ([key]: ArrayEntry) => key;
Expand Down Expand Up @@ -99,7 +101,7 @@ export function groupByRollup(
const statistics: Statistics = {
globalAggregate: NaN,
};
const reductionMap = factTable.reduce((p: HierarchyOfMaps, n, index) => {
const reductionMap: HierarchyOfMaps = factTable.reduce((p: HierarchyOfMaps, n, index) => {
const keyCount = keyAccessors.length;
let pointer: HierarchyOfMaps = p;
keyAccessors.forEach((keyAccessor, i) => {
Expand Down Expand Up @@ -140,14 +142,13 @@ function getRootArrayNode(): ArrayNode {
[PATH_KEY]: [] as number[],
};
Object.assign(bootstrap, { [PARENT_KEY]: bootstrap });
const result: ArrayNode = bootstrap as ArrayNode;
return result;
return bootstrap as ArrayNode;
Copy link
Member

Choose a reason for hiding this comment

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

This is just a clean up on the PR, but seriously looking at that getRootArrayNode function, it doesn't return strictly an ArrayNode, the STATISTICS_KEY and SORT_INDEX_KEY are not available there.
It's possible that for the root note these values are ignored in the current code, but that doesn't mean that we can easily remember that fact when refactoring/reworking on that file. It can leads to bug if we believe that also the root node is of type ArrayNode
Fixing the type here can go in two direction:

  • add missing keys in the bootstrap object with some default values, declare the bootstrap as Exclude<ArrayNode, parent
  • relax the ArrayNode type for the missing keys
  • create a RootArrayNode type without these missing keys

This task doesn't need to be handled in this PR, but it's just an example of why ts ignoring or putting as can deal to issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, good points!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pondering... the possible issue with points 1 and 3 is that it'll no longer be possible to model the entire tree as having homogeneous type; I'm sure it's solvable with type guards and such, with some more work.

With option 2, it'd relax the type for all uses, and I wouldn't want to water it up.

I think adding the missing properties to this root node would be a compact, local solution, but it'd still need the as assertion, because the [PARENT_KEY] prop demands an ArrayNode.

So my current plan is to see what I can do locally (eg. just adding the missing props but leaving in place the as) and continue exploring and likely, discussing, outside this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK the current solution is, use Omit to exclude PARENT_KEY in the assignment:

function getRootArrayNode(): ArrayNode {
  const children: HierarchyOfArrays = [];
  const bootstrap: Omit<ArrayNode, typeof PARENT_KEY> = {
    [AGGREGATE_KEY]: NaN,
    [DEPTH_KEY]: NaN,
    [CHILDREN_KEY]: children,
    [INPUT_KEY]: [] as number[],
    [PATH_KEY]: [] as number[],
    [SORT_INDEX_KEY]: 0,
    [STATISTICS_KEY]: { globalAggregate: 0 },
  };
  return { ...bootstrap, [PARENT_KEY]: bootstrap } as ArrayNode; // TS doesn't yet handle bootstrapping but the `Omit` above retains guarantee for all props except `[PARENT_KEY`
}

This way, there's still a need for as, because TS can't type-bootstrap a self-referential object, but it's way better, because should anything in ArrayNode change, the bootstrap object must reflect that. So the only part where the type checker is asked to trust the human is the presence of PARENT_KEY which is a local, single-line assignment right there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

/** @internal */
export function mapsToArrays(root: HierarchyOfMaps, sorter: NodeSorter): HierarchyOfArrays {
const groupByMap = (node: HierarchyOfMaps, parent: ArrayNode) =>
Array.from(
export function mapsToArrays(root: HierarchyOfMaps, sorter: NodeSorter | null): HierarchyOfArrays {
const groupByMap = (node: HierarchyOfMaps, parent: ArrayNode) => {
const items = Array.from(
node,
([key, value]: [Key, MapNode]): ArrayEntry => {
const valueElement = value[CHILDREN_KEY];
Expand All @@ -168,12 +169,15 @@ export function mapsToArrays(root: HierarchyOfMaps, sorter: NodeSorter): Hierarc
);
return [key, newValue];
},
)
.sort(sorter)
.map((n: ArrayEntry, i) => {
entryValue(n).sortIndex = i;
return n;
}); // with the current algo, decreasing order is important
);
if (sorter !== null) {
items.sort(sorter);
}
return items.map((n: ArrayEntry, i) => {
entryValue(n).sortIndex = i;
return n;
});
}; // with the current algo, decreasing order is important
const tree = groupByMap(root, getRootArrayNode());
const buildPaths = ([, mapNode]: ArrayEntry, currentPath: number[]) => {
const newPath = [...currentPath, mapNode[SORT_INDEX_KEY]];
Expand Down
13 changes: 7 additions & 6 deletions src/chart_types/partition_chart/layout/utils/sunburst.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@ import { ArrayEntry, childrenAccessor, HierarchyOfArrays } from './group_by_roll

/** @internal */
export function sunburst(
nodes: HierarchyOfArrays,
outerNodes: HierarchyOfArrays,
areaAccessor: (e: ArrayEntry) => number,
{ x0, y0 }: Origin,
{ x0: outerX0, y0: outerY0 }: Origin,
clockwiseSectors: boolean,
specialFirstInnermostSector: boolean,
heightStep: number = 1,
): Array<Part> {
const result: Array<Part> = [];
const laySubtree = (nodes: HierarchyOfArrays, { x0, y0 }: Origin, depth: number) => {
Expand All @@ -36,14 +37,14 @@ export function sunburst(
const index = clockwiseSectors ? i : nodeCount - i - 1;
const node = nodes[depth === 1 && specialFirstInnermostSector ? (index + 1) % nodeCount : index];
const area = areaAccessor(node);
result.push({ node, x0: currentOffsetX, y0, x1: currentOffsetX + area, y1: y0 + 1 });
result.push({ node, x0: currentOffsetX, y0, x1: currentOffsetX + area, y1: y0 + heightStep });
const children = childrenAccessor(node);
if (children && children.length) {
laySubtree(children, { x0: currentOffsetX, y0: y0 + 1 }, depth + 1);
if (children.length > 0) {
laySubtree(children, { x0: currentOffsetX, y0: y0 + heightStep }, depth + 1);
}
currentOffsetX += area;
}
};
laySubtree(nodes, { x0, y0 }, 0);
laySubtree(outerNodes, { x0: outerX0, y0: outerY0 }, 0);
return result;
}
19 changes: 12 additions & 7 deletions src/chart_types/partition_chart/layout/utils/treemap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,20 +101,25 @@ export function treemap(
areaAccessor: (e: ArrayEntry) => number,
topPaddingAccessor: (e: ArrayEntry) => number,
paddingAccessor: (e: ArrayEntry) => number,
{ x0, y0, width, height }: { x0: number; y0: number; width: number; height: number },
{
x0: outerX0,
y0: outerY0,
width: outerWidth,
height: outerHeight,
}: { x0: number; y0: number; width: number; height: number },
): Array<Part> {
if (nodes.length === 0) return [];
// some bias toward horizontal rectangles with a golden ratio of width to height
const vertical = width / GOLDEN_RATIO <= height;
const independentSize = vertical ? width : height;
const vertical = outerWidth / GOLDEN_RATIO <= outerHeight;
const independentSize = vertical ? outerWidth : outerHeight;
const vectorElements = bestVector(nodes, independentSize, areaAccessor);
const vector = vectorNodeCoordinates(vectorElements, x0, y0, vertical);
const vector = vectorNodeCoordinates(vectorElements, outerX0, outerY0, vertical);
const { dependentSize } = vectorElements;
return vector
.concat(
...vector.map(({ node, x0, y0, x1, y1 }) => {
const childrenNodes = entryValue(node)[CHILDREN_KEY];
if (!childrenNodes || !childrenNodes.length) {
if (childrenNodes.length === 0) {
return [];
}
const fullWidth = x1 - x0;
Expand Down Expand Up @@ -148,8 +153,8 @@ export function treemap(
topPaddingAccessor,
paddingAccessor,
vertical
? { x0, y0: y0 + dependentSize, width, height: height - dependentSize }
: { x0: x0 + dependentSize, y0, width: width - dependentSize, height },
? { x0: outerX0, y0: outerY0 + dependentSize, width: outerWidth, height: outerHeight - dependentSize }
: { x0: outerX0 + dependentSize, y0: outerY0, width: outerWidth - dependentSize, height: outerHeight },
),
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ import {
groupByRollup,
mapEntryValue,
mapsToArrays,
Sorter,
} from '../utils/group_by_rollup';

export function getHierarchyOfArrays(
rawFacts: Relation,
valueAccessor: ValueAccessor,
groupByRollupAccessors: IndexedAccessorFn[],
sorter: Sorter | null = childOrders.descending,
): HierarchyOfArrays {
const aggregator = aggregators.sum;

Expand All @@ -52,6 +54,6 @@ export function getHierarchyOfArrays(
// size as data value vs size as number of pixels in the rectangle
return mapsToArrays(
groupByRollup(groupByRollupAccessors, valueAccessor, aggregator, facts),
aggregateComparator(mapEntryValue, childOrders.descending),
sorter && aggregateComparator(mapEntryValue, sorter),
);
}
Loading