-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
improve robustness of circular Sankey #3932
Conversation
package.json
Outdated
@@ -66,7 +66,7 @@ | |||
"d3-force": "^1.0.6", | |||
"d3-hierarchy": "^1.1.8", | |||
"d3-interpolate": "1", | |||
"d3-sankey-circular": "0.33.0", | |||
"d3-sankey-circular": "[email protected]:antoinerg/d3-sankey-circular.git#8943bbd049c4651dbb230aa0e55ee8aad5d85afd", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to make a PR to d3-sankey-circular
. Maybe the author will have an opinion and/or tips about your patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I sent a PR upstream: tomshanley/d3-sankey-circular#36 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. That's frustrating. No news from the d3-sankey-circular
maintainers. We might have to make a new package out of your fork e.g. @plotly/d3-sankey-circular
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! I will make a release today
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5fed1a5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One baseline was altered by the hotfix and unfortunately, it is for the worse... The layout was better before. On average, however, the difference should not be appreciable. |
Ok, interesting. Do you have an idea of how common data similar to For what it's worth, I'm ok with this patch. Deteriorating some cases while fixing cases that were 100% broken sounds like a win to me, but will I don't think we should include this in a patch release; this might have to wait for 1.49.0 |
I really can't say. I haven't seen that many Sankey diagrams with circular links in the wild.
It is definitely a win. That being said, down the road, it might be worth investigating ways to improve the default position of nodes. |
Nicely done 💃 ! |
Closes #3813
As reported in #3813:
1st dataset:
Before: https://codepen.io/antoinerg/pen/BebjwJ?editors=0010 (hangs indefinitely)
After: https://codepen.io/antoinerg/pen/arMvXX
2nd dataset:
Before: https://codepen.io/joetristano/pen/wbKjbo (hangs indefinitely)
After: https://codepen.io/antoinerg/pen/eaXJEo
Minified bundle: https://40395-45646037-gh.circle-artifacts.com/0/dist/plotly.min.js