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

Update Sankey algorithm to match latest d3-sankey #3548

Closed
philippjfr opened this issue Mar 9, 2019 · 9 comments · Fixed by #4707
Closed

Update Sankey algorithm to match latest d3-sankey #3548

philippjfr opened this issue Mar 9, 2019 · 9 comments · Fixed by #4707
Labels
good first issue An issue well suited to a new contributor type: enhancement Minor feature or improvement to an existing feature

Comments

@philippjfr
Copy link
Member

Michael Bostock has made some major improvements to the overlap avoidance algorithms in d3-sankey over the last week. Since the code in holoviews is a fairly straightforward Python port of that code it should be reasonably straightforward to update our code to match. Here's a link to the diff for all the updates.

@philippjfr philippjfr added good first issue An issue well suited to a new contributor type: enhancement Minor feature or improvement to an existing feature labels Mar 9, 2019
@Yura52
Copy link
Contributor

Yura52 commented Oct 28, 2020

I'd like to work on this. Are there any good examples where the improvements after the update can be observed?

@Yura52
Copy link
Contributor

Yura52 commented Oct 30, 2020

@philippjfr @jbednar

Hi! I am almost done with the implementation, just want to double check if there is still interest in it.

@philippjfr
Copy link
Member Author

@Yura52 Amazing! We are definitely still interested. Would love to see a PR.

@Yura52
Copy link
Contributor

Yura52 commented Nov 17, 2020

Sorry for the delay. The implementation is finished. However, I cannot find data where improvements can be observed. Do you have any examples?

The screenshot is based on this example. 1.13.5. on the left, my implementation on the right. The first row shows nodes, the second row shows edges. The "problem" is that the final sankey plot is absolutely the same in both cases and looks like in the gallery example.

So it is either my mistake or "bad" data. The only thing that my implementation changes is how the graph is filled (lines 48-52 here), maybe this is not enough.

Screenshot 2020-11-17 at 23 23 18

@Yura52
Copy link
Contributor

Yura52 commented Nov 26, 2020

@philippjfr I can create a PR, but I would prefer to make sure that the results I see are expected. Any additional infomation will be helpful.

@philippjfr
Copy link
Member Author

Sorry for not getting back to you before. I'd say open the PR and then I can play around with it and give feedback.

@jnettels
Copy link

jnettels commented Mar 8, 2021

Hello there, just an occasional user here. I was curious about the progress and saw the images in the PR.

As another user mentioned here #3501 (comment) the biggest issue with holoviews sankey currently seems to be how nodes are aligned towards the center, creating unnecessary overlap of edges and nodes.
I wonder if this PR is a chance to change this behaviour or (maybe instead) expose some parameters to control this behaviour.

Regards, and thanks for your work!

@Yura52
Copy link
Contributor

Yura52 commented Mar 9, 2021

@jnettels Hi! Have you seen the latest PR update ? What do you think of the (possibly) upcoming changes?

P.S. I would suggest leaving comments in that PR since it contains screenshots and some other information.

Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue An issue well suited to a new contributor type: enhancement Minor feature or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants