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

Fix dagre.js for abnormal wide edges #1246

Closed
wants to merge 1 commit into from

Conversation

seanshpark
Copy link

This will dagre.js for abnormal wide edges

This will dagre.js for abnormal wide edges
@seanshpark
Copy link
Author

this is to fix #864 issue

@@ -1591,7 +1591,7 @@ dagre.layout = (graph, layout) => {
}
const label = g.node(v).label;
if (min !== Number.POSITIVE_INFINITY && label.borderType !== borderType) {
xs[v] = Math.max(xs[v], min);
xs[v] = Math.min(xs[v], min);
Copy link
Author

Choose a reason for hiding this comment

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

this will choose smallest width

Copy link

Choose a reason for hiding this comment

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

@seanshpark @lutzroeder

How about the following code?

                            let minOut = Number.POSITIVE_INFINITY;
                            let minIn = Number.POSITIVE_INFINITY;
                            for (const e of blockG.node(v).out) {
                                minOut = Math.min(minOut, xs[e.w] - e.label);
                            }
                            for (const e of blockG.node(v).in) {
                                minIn = Math.min(minIn, xs[e.w] - e.label);
                            }
                            const label = g.node(v).label;
                            if (minOut !== Number.POSITIVE_INFINITY && minIn !== Number.NEGATIVE_INFINITY && label.borderType !== borderType) {
                                if (Math.abs(xs[v] - minOut) < Math.abs(xs[v] - minIn)) {
                                    xs[v] = Math.max(xs[v], minOut);
                                }else {
                                    xs[v] = Math.max(xs[v], minIn);
                                }
                            }
model before after
yolov5n yolov5n onnx new_yolov5n onnx
sample.zip saved_model pbtxt new_saved_model pbtxt

Copy link
Author

Choose a reason for hiding this comment

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

How about the following code?

This looks better !

@@ -1740,7 +1740,7 @@ dagre.layout = (graph, layout) => {
for (const [v, x] of Object.entries(xs)) {
const halfWidth = g.node(v).label.width / 2;
max = Math.max(x + halfWidth, max);
min = Math.min(x - halfWidth, min);
min = Math.min(Math.max(0, x - halfWidth), min);
Copy link
Author

Choose a reason for hiding this comment

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

some x - halfWidth goes to minus, which will make below const width = max - min; larger.

@lutzroeder
Copy link
Owner

lutzroeder commented Mar 19, 2024

@seanshpark this is changing how other models are rendered? Is there a good way to reason pros vs. cons?

sample.zip

Before:

1

After:

2

@seanshpark
Copy link
Author

Is there a good way to reason pros vs. cons?

@ lutzroeder, thanks for looking into this !
I'll take a look at the sample model :)

@seanshpark
Copy link
Author

@lutzroeder , as not knowing much about algorithms in dagre, my wild guess is that the end node of the example graph tries to go to the center of the layout (the minimum diff from center?)

I'll try to study what the algorithm does and find a way for this.
If the example is important, you can just ignore this PR,
or if most of the well know models work, I would be glad if you can land this change.

Anyway, I'll try to find something better.

@lutzroeder lutzroeder force-pushed the main branch 5 times, most recently from 66d40fe to 19e4123 Compare March 31, 2024 17:34
@seanshpark
Copy link
Author

Close this for now and reopen when change is ready.

@seanshpark seanshpark closed this Mar 31, 2024
@seanshpark
Copy link
Author

I couldn't repoen this so posted another #1254

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 this pull request may close these issues.

3 participants