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

d3-sankey-circular #3406

Merged
merged 41 commits into from
Feb 14, 2019
Merged

d3-sankey-circular #3406

merged 41 commits into from
Feb 14, 2019

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Jan 7, 2019

Initial integration of d3-sankey-circular to close #2636

This branch is based off PR #3355 which brings d3-sankey to version 0.7.1.

When detecting circular links, we swap from d3-sankey(https://github.com/d3/d3-sankey) to d3-sankey-circular (https://github.com/tomshanley/d3-sankey-circular). This is possible because d3-sankey-circular has a similar API to d3-sankey 0.7.x and this is why PR #3355 was an important stepping stone.

Here are example baselines:

To do:

  • Add dragging and a force model
  • Links are not rectangles anymore but path (so we don't have a fill property, but only stroke)
  • Style link on hover

@antoinerg antoinerg changed the base branch from sankey-d3-sankey-0.7 to master January 7, 2019 22:47
package.json Outdated
"alpha-shape": "^1.0.0",
"array-range": "^1.0.1",
"canvas-fit": "^1.5.0",
"color-normalize": "^1.3.0",
"convex-hull": "^1.0.3",
"country-regex": "^1.1.0",
"d3": "^3.5.12",
"d3-sankey": "git://github.com/antoinerg/d3-sankey.git#4f37ed8d3578b545a8569ecd74583f373768e900",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimal changes over latest d3-sankey d3/d3-sankey@master...antoinerg:fix-large-padding

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. So we no longer need https://github.com/plotly/d3-sankey ? I thought i read that we made changes in our @plotly/d3-sankey that got rejected by d3/d3-sankey.

Copy link
Contributor Author

@antoinerg antoinerg Jan 15, 2019

Choose a reason for hiding this comment

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

Those changes were minimal. One of the biggest was that we generated the SVG path for the links in it. As you found out in this PR, we can do this operation in plotly.js instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok that make sense. Do you think we have a shot at getting

d3/d3-sankey@master...antoinerg:fix-large-padding

merged into d3/d3-sankey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There hasn't been anything merged into master in d3/d3-sankey since Jul 13, 2017 so I'm not sure... I opened a PR almost a month ago d3/d3-sankey#63 and there has been no activity since then.

package.json Outdated
"alpha-shape": "^1.0.0",
"array-range": "^1.0.1",
"canvas-fit": "^1.5.0",
"color-normalize": "^1.3.0",
"convex-hull": "^1.0.3",
"country-regex": "^1.1.0",
"d3": "^3.5.12",
"d3-sankey": "git://github.com/antoinerg/d3-sankey.git#4f37ed8d3578b545a8569ecd74583f373768e900",
"d3-sankey-circular": "git://github.com/antoinerg/d3-sankey-circular.git#a298acf674f0a9c7158243d45814cd8060dad728",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimal changes over latest d3-sankey-circular tomshanley/d3-sankey-circular@master...antoinerg:support-update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those changes got merged in 🚀 !

@antoinerg
Copy link
Contributor Author

Here's a Codepen to generate random mocks with circular links: https://codepen.io/anon/pen/LMBZbK

You can play around with N, L and targetOffset in the source code.

@AryehGielchinsky
Copy link

AryehGielchinsky commented Jan 13, 2019

Hi @antoinerg, thank you for your work bringing circular Sankey to plotly. I'm beginner/intermediate at python, beginner at plotly, and don't know any JavaScript. Is there any way I could help?

@antoinerg
Copy link
Contributor Author

antoinerg commented Jan 14, 2019

Hello @AryehGielchinsky and thank you for your interest in plotly.js. Sankey is undergoing majors changes as we're implementing several new features. Getting feedback from the community is always useful.

What's your use case for circular Sankey? Do you have interesting datasets with circularity that we could use as examples?

@antoinerg
Copy link
Contributor Author

hovers appear in the center of circular loops.

Good ! For forward links the bounding box center is always on the link, but for circular links it would be great to move the hover box anchor to the center of the back stretch.

@alexcjohnson Is the following what you had in mind? I am not sure what part is the "back stretch".

sankey_fixed_hover

@alexcjohnson
Copy link
Collaborator

@alexcjohnson Is the following what you had in mind? I am not sure what part is the "back stretch".

Yep, you got it exactly as I had in mind! https://en.wikipedia.org/wiki/Backstretch

@AryehGielchinsky
Copy link

Hi @antoinerg unfortunately I wont be able to share Sankey diagrams based on my company's datasets.

I look forward to the completion of this project, I think it will make plotly's sankey diagrams much more useful. Keep up the good work!

@etpinard
Copy link
Contributor

etpinard commented Jan 21, 2019

@antoinerg this thing is starting to look great. Here's an updated TODO list:

  • ask the d3-sankey-circular author to publish a new version to npm, update our deps accordingly
  • merge the changes from your d3-sankey fork to @plotly/d3-sankey, update our deps accordingly
  • add a few jasmine tests to 🔒 down sankey.update()

@antoinerg
Copy link
Contributor Author

antoinerg commented Jan 22, 2019

In commit 9b36090, I 🔒 down sankey.update() and fix a little bug that I discovered in its implementation in d3-sankey-circular: it wouldn't force circular links to go from top to bottom on the first invocation of update(). Because we make the call to update() in the animation loop over and over, this error on the first call was not perceivable to the user. Anyway, this is now fixed.

I will submit PR upstream (edit: done here tomshanley/d3-sankey-circular#29). BTW, its maintainer @tomshanley said he could publish an npm package as soon as he gets back from holiday (tomshanley/d3-sankey-circular#28 (comment)).

@antoinerg
Copy link
Contributor Author

@etpinard Tomorrow I intend to create/update our own forks for both d3-sankey and d3-sankey-circular and publish npm packages for them. Was updating our deps the only remaining thing blocking this PR?

@etpinard
Copy link
Contributor

Was updating our deps the only remaining thing blocking this PR?

Yes. Your new tests look good!

@@ -0,0 +1,65 @@
{
"data": [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool mock!

expect(sankey.nodePadding()).toEqual(10, 'incorrect nodePadding');
});
// Update links
var updatedGraph = sankey.update(graph);
Copy link
Contributor

@etpinard etpinard Jan 24, 2019

Choose a reason for hiding this comment

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

Actually, can we make these tests use restyle instead of (the internal) sankey.update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, can we make these tests use restyle instead of (the internal) sankey.update?

I don't think we can do that right now because we don't have any attributes to control the position of the nodes (so no restyle call can change the position of the nodes). Right now, sankey.update is only called when nodes are dragged around with the mouse.

Maybe tests using restyle could be included in a future PR implementing attributes node.x and node.y? Or maybe I should add those attributes here right now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha I see. Good point.

Can you add (just) one restyle test that update circular to non-circular node and links data? Or better yet, one react test that goes from one circular mock to a non-circular one?

Copy link
Contributor

Choose a reason for hiding this comment

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

One last TODO ⬆️

@etpinard
Copy link
Contributor

Amazing work!

💃 💃 💃

@antoinerg antoinerg merged commit 30eaa3a into master Feb 14, 2019
@antoinerg antoinerg deleted the sankey-circular branch March 26, 2019 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Sankey - support circular references
4 participants