You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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:
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.
The text was updated successfully, but these errors were encountered:
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 branchshould be
-->below
should be ordered to the bottom of the graph.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: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.The text was updated successfully, but these errors were encountered: