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

Original ordering is not respected in some cases when cross-count is already 0 #336

Open
scottyantipa opened this issue Nov 19, 2022 · 0 comments

Comments

@scottyantipa
Copy link

I have found a few cases where the ordering algorithm will permute the order of nodes unnecessarily. It does not further minimize edge crossings, because they are already at 0 with the given node ordering, but it does change the final layout in a way that goes against the provided node ordering.

Here's a reproduction using test/console.html. You can see that the branch should be --> below should be ordered to the bottom of the graph.
console-repro-bad

This seems to happen because order/index.js will run the cross minimization algorithm regardless of if the cross count is already 0. So the nodes get permuted for no reason, and just ends up in the end with a cross minimization of still 0. I tested this by adding an early return if cross minimization is 0 and get the desired result:
console-show-fix

The code for the fix is in #335. Unfortunately this breaks compound graph layout which I show in the PR.

As a follow up note, this has perf implications because it isn't necessary to run the cross minimization iterations at all if it's already at 0 -- unless the cross minimization iterations are required for compound graphs, which could be the case since it breaks the compound graph layout. Another option would be to add an early return within the for-loop, eg to check if the cross minimization is 0 in each iteration and return early. I've noticed that this does not break compound graph layout, I guess because it allows us to run sweepLayerGraph at least 1.

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

Successfully merging a pull request may close this issue.

1 participant