Inconsistent layout with nodePadding

[techniq]

Hi @tomshanley,

I'm looking to use d3-sankey-circular to provide a <Sankey> layout component for vx as the support for circular references is great, but when comparing non-circular layouts that d3-sankey produces, d3-sankey-circular's layouts are not as optimal.

One example I've run across is with the handling of nodePadding. With d3-sankey, the layout does not appear to jump around as the nodePadding goes up and down...

...but with d3-sankey-circular, it's rather eradicate:

Here is the codesandbox that demonstrates the issue.

[tomshanley]

Hi - thanks for the evaluating this library! There is an issue with sankeycircular compared to the original sankey library. sankeycircular has a few more layout 'optimisations', such as trying to avoid overlap of nodes with links that span multiple columns (it'll try to shift nodes to more optiimal heights), and also shifting links within the node's height to avoid overlaps with other links. In addition, I've not optimised these layouts when there's no circular links. These combined, mean that each change of a variable is more likely to produce a quite different layout, as you've seen, compared to the original d3.sankey which has less optimisations, which makes it more consistent.

I need to improve sankeycircular in this regards (perhaps defaulting back to nearly all the original sankey functions where's no circular links detected). Also looking at your example, there's still some sub-optimal layouts (like where the Nuclear node is ending up, as it looks like the optimisation towards straighter links is beating the overlaps)

Also, I am considering how to update a sankey after its drawn (based on different link values, but it could be extended to other variables like nodePadding) so that the layout remains close to the originally drawn sankey (see issue 20). This should help.

Another factor is that I want to drop a hard coded nodePadding pixel value, and replace it with the nodePaddingRatio (something like the padding ratios used in the ordinal axis layouts) so that is scales when the node values update.

I'm not going to be able to fix this soon I'm afraid, as I'll need to review most parts of the layouts.

I work around would be select sankeyCircular only when you have known circular links, which isn't ideal I know

[techniq]

Thanks for the detailed response. I expected to have some differences between the two libraries and have been considering supporting both (kind of how I did within the codesandbox). I was trying to minimize the API of the react components as much as possible but it might make sense to have <Sankey> (based on d3-sankey) and <CircularSankey> (based on d3-sankey-circular) especially since they are likely to have different props/apis (especially as your library evolves). Thanks again for the library and I'll let you know how the vx component goes. I see you on vx's slack channel so we can chat there sometimes as well. Might be a few weeks before I get the PR submitted depending on other priorities.

…

On Thu, Jun 14, 2018, 4:20 PM Tom Shanley ***@***.***> wrote: Hi - thanks for the evaluating this library! There is an issue with sankeycircular compared to the original sankey library. sankeycircular has a few more layout 'optimisations', such as trying to avoid overlap of nodes with links that span multiple columns (it'll try to shift nodes to more optiimal heights), and also shifting links within the node's height to avoid overlaps with other links. In addition, I've not optimised these layouts when there's no circular links. These combined, mean that each change of a variable is more likely to produce a quite different layout, as you've seen, compared to the original d3.sankey which has less optimisations, which makes it more consistent. I need to improve sankeycircular in this regards (perhaps defaulting back to nearly all the original sankey functions where's no circular links detected). Also looking at your example, there's still some sub-optimal layouts (like where the Nuclear node is ending up, as it looks like the optimisation towards straighter links is beating the overlaps) Also, I am considering how to update a sankey after its drawn (based on different link values, but it could be extended to other variables like nodePadding) so that the layout remains close to the originally drawn sankey (see issue 20). This should help. Another factor is that I want to drop a hard coded nodePadding pixel value, and replace it with the nodePaddingRatio (something like the padding ratios used in the ordinal axis layouts) so that is scales when the node values update. I'm not going to be able to fix this soon I'm afraid, as I'll need to review most parts of the layouts. I work around would be select sankeyCircular only when you have known circular links, which isn't ideal I know — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK1RIIxTvPfgaITWz5hjQiP-xL4Tfb2ks5t8sWogaJpZM4Uoefx> .

[tomshanley]

Yeah, I know its not ideal to have to support two libraries. Admittedly, I could include the d3-sankey, and sankeyCircular does the test for circular links and then proceed accordingly. I'll try that next time I've got some headspace for this project.

[techniq]

That might be a descent approach (not to burden you more though). We could stay synced based on your API and as time goes on we could drop d3-sankey. I know including d3-sankey along with d3-sankey-circular will inflate bundle sizes so not sure if you should add it for all users of the library or creating separate components and hoping they tree shake away...

…

On Thu, Jun 14, 2018, 7:27 PM Tom Shanley ***@***.***> wrote: Yeah, I know its not ideal to have to support two libraries. Admittedly, I could include the d3-sankey, and sankeyCircular does the test for circular links and then proceed accordingly. I'll try that next time I've got some headspace for this project. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK1RCaP8zWfpcAuiQl_TbdO672XtpHTks5t8vFJgaJpZM4Uoefx> .

[tomshanley]

i may just incorporate the parts that are different and switch based on the presence of circular links. probably wouldn't inflate too much, but I will do a test. initially it may be more, but I'm sure I could find more opportunities to rationalise.

[techniq]

Sounds good

…

On Thu, Jun 14, 2018, 7:56 PM Tom Shanley ***@***.***> wrote: i may just incorporate the parts that a different and switch based on the presence of circular links. probably wouldn't inflate too much, but I will do a test. initially it may be more, but I'm just I could find more opportunities to rationalise. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#24 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAK1ROSCcONENeZNZ-rD6hdsCVLWJ8WPks5t8vg5gaJpZM4Uoefx> .