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

Hierarchy layouts should allow non-root nodes as input? #91

Open
ernest-okot opened this issue Jun 3, 2017 · 5 comments
Open

Hierarchy layouts should allow non-root nodes as input? #91

ernest-okot opened this issue Jun 3, 2017 · 5 comments

Comments

@ernest-okot
Copy link

ernest-okot commented Jun 3, 2017

When a descendant node (i.e depth != 0) is 'partitioned', it's children will be be positioned inaccurately.

screenshot from 2017-06-04 02-16-31
Here the 'world' node is 'partitioned'

partition()(world)

screenshot from 2017-06-04 02-16-55
Here the 'africa' node is partitioned, because the 'africa' node depth > 0, the layout gets broken

partition()(africa)
ernest-okot added a commit to ernest-okot/d3-hierarchy that referenced this issue Jun 3, 2017
ernest-okot added a commit to ernest-okot/d3-hierarchy that referenced this issue Jun 3, 2017
@curran
Copy link

curran commented Jun 4, 2017

Would you be able to share a running code example that demonstrates this behavior? Thank you.

@ernest-okot
Copy link
Author

ernest-okot commented Jun 4, 2017

sure!

i've pieced together a fiddle https://jsfiddle.net/devacholi/k3m83ofn/3/

const data = [
  {"label": 'World',}, 
  {"label": 'Europe', "parent": 'World'}, 
  {"label": 'Portugal', "parent": 'Europe', "value": 30}, 
  {"label": 'Spain', "parent": 'Europe', "value": 120}, 
  {"label": 'South America', "parent": 'World',"value": 320},
  {"label": 'Africa',"parent": 'World'},
  {"label": 'Uganda', "parent": 'Africa',}, 
  {"parent": 'Uganda',"label": 'Jinja',"value": 20,},
  { "parent": 'Uganda', "label": 'Kampala', "value": 40,},
  { "label": 'Kenya', "parent": 'Africa', "value": 70,}, 
]

const stratify = d3.stratify()
  .id(d => d.label)
  .parentId(d => d.parent);

const layout = d3.partition()

const root = stratify(data)
	.sum(d => d.value)
  .sort((a,b) => a.value - b.value);

// node.depth = 0
const okRectangles = layout(root)
  .descendants()

console.log('expecting 0, got ' +  okRectangles.filter(r => r.y1 > 1).length)
// expecting 0, got 0

// root.children[0].depth = 1
const badRectangles = layout(root.children[0])
  .descendants()

console.log('expecting 0, got ' + badRectangles.filter(r => r.y1 > 1).length)
// expecting 0, got 2

ernest-okot added a commit to freelyformd/charts that referenced this issue Jun 4, 2017
Partition function that fixes d3/d3-hierarchy#91
@mbostock
Copy link
Member

mbostock commented Jun 5, 2017

Per #80 (comment), this is a documented limitation; the hierarchy layouts take a root node as input, not any node. To quote the README:

Before you can compute a hierarchical layout, you need a root node.

And:

node.depth - zero for the root node, and increasing by one for each descendant generation.

You can use node.copy to create a new root from an existing node’s subtree.

My longer-term goal for this module is that the hierarchy layouts no longer mutate the input data (#62). That would make it easier to avoid the call to node.copy before computing the layout. But I don’t think it’s worth fixing this issue in just one of the layouts, when the other layouts have the same problem. It would be reasonable to add some text to the README to clarify this limitation and the recommended workaround in the interim.

@mbostock mbostock changed the title Partition layout inaccurate when root depth is not zero Hierarchy layouts should allow non-root nodes as input? Jun 5, 2017
@mbostock
Copy link
Member

mbostock commented Jun 5, 2017

Retitled to generalize this issue.

@mbostock mbostock reopened this Jun 5, 2017
@ernest-okot
Copy link
Author

ernest-okot commented Jun 5, 2017

Thanks for the explanation

Didn't know about node.copy. Not sure if it would be of help if I wanted to maintain the parent relationship of the new root

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants