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

Feature request: Sankey - support circular references #2636

Closed
ChristopherBull opened this issue May 15, 2018 · 7 comments · Fixed by #3406
Closed

Feature request: Sankey - support circular references #2636

ChristopherBull opened this issue May 15, 2018 · 7 comments · Fixed by #3406
Assignees

Comments

@ChristopherBull
Copy link

Circular references for Sankey graphs are not currently supported:
ERROR: Circularity is present in the Sankey data. Removing all nodes and links.

This would be a fantastically useful optional ability for Plotly.js.

I have found an example that uses d3.js here:
http://bl.ocks.org/tomshanley/11392275
...but it is not as developer-friendly as the Plotly.js library.

@ChristopherBull
Copy link
Author

A bit of digging, this is the fork of d3 that supports circular sankeys:
https://github.com/tomshanley/d3-sankey-circular/
https://www.npmjs.com/package/d3-sankey-circular

Trying to find a way if I can override Plotly's internals to use:
var sankey = d3.sankeyCircular();
...rather than than d3's default:
var sankey = d3.sankey();

Any help appreciated, even if it is a shim on my end and not officially supported in Plotly.

@alexcjohnson
Copy link
Contributor

alexcjohnson commented May 15, 2018

This would definitely be a great feature to have. It's a tricky problem though, and we'd really like to at least try for a friendlier-looking layout, preserving line thickness and avoiding overlaps where possible. Note that plotly.js already uses our own fork of d3-sankey: https://github.com/plotly/d3-sankey, mainly because we weren't happy with how the original handles some common edge cases like very sharply curved links.

So the ideal development path from our standpoint would be to make a PR to that repo and use the result in a PR to plotly.js that removes the circularity check. If you're interested in trying it out, you can develop in both repos simultaneously using npm link.

As an aside, there has been further development in the main d3-sankey repo https://github.com/plotly/d3-sankey - would be nice to take a look at that, see if it's worthwhile pulling those changes into our own fork. Maybe they even obviate the need for some of the changes we made? I doubt the original repo would accept our changes as a PR back though (edit: they explicitly rejected it in fact), as we change the SVG structure of the links kind of fundamentally, from a path whose stroke is the link width to a filled path with no stroke. That said the circular fork you reference above obviously uses a filled path for loops just like we do...

@alexcjohnson
Copy link
Contributor

BTW I looked at the updates to d3-sankey since our fork - they've added a few features, and merged one of @monfera 's fixes, doesn't look like there should be any problem updating our fork but also no particular rush.

@egberts
Copy link

egberts commented Jun 30, 2018

This guy... wrote about the curving of a circular reference of (true) Sankey diagram.

https://pure.tue.nl/ws/files/46963135/774491-1.pdf

@olveirap
Copy link

olveirap commented Oct 9, 2018

@CaffeinatedAndroid did you find a way of overriding Plotly's internals? I'm also looking right now to do a Sankey diagram that supports cycles and this feature would be great.

@ChristopherBull
Copy link
Author

Sorry, no - I ended up settling for the Git fork I mentioned earlier:
https://github.com/tomshanley/d3-sankey-circular

This works just fine for now, but with it being a fork, it will of course not keep up with Plot.ly updates - so hopefully I have no need to update the version of my Plot.ly libs.

@ChristopherBull
Copy link
Author

Super 🥇

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

Successfully merging a pull request may close this issue.

5 participants