Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Added a Tutte Embedding Layout for Graphs #38762
base: develop
Are you sure you want to change the base?
Added a Tutte Embedding Layout for Graphs #38762
Changes from all commits
bea87db
43ae7d2
488449b
300f3b5
152acdd
ff6372d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Be more precise on the connectivity requirement. Is it exactly or at least 3 ?
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.
Any graph that is k-connected for k$\geq$ 3 is 3-connected. For example, a 5-connected graph is 3-connected.
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.
isn't it possible to find an embedding without specifying the external face ?
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.
In order to calculate the embedding, an external face must be specified. Any subgraph of the graph that is a cycle will work as an external face. It would be possible to find an arbitrary external face automatically if none is specified, but it's unclear to me how one would be chosen from the available options. Also, that functionality is outside the scope of my original intention.
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.
Consider tha following:
You get a short cycle
C
, and this cycle is a face.This can of course be done in a follow up PR, but this seems quite simple.
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.
this line requires 2 more spaces indentation. See other methods for examples.
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.
In the other methods, it seems like the inputs have two spaces of indentation for an input element which requires more than a line of explanation. Is that not correct?
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.
This doctest is unstable. I obtain different results on my laptop (macOS...)
You should find another test that is more stable. Printing vertex positions is never a good idea.
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.
In many of the other layout examples, they include this sort of example. However, I've removed it from this function.
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.
this test is failing on my laptop
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.
I believe this is a quirk of WSL, it should be fixed now.
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.
Why do you need a copy of the graph?
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.
I was copying another layout function, but you're right I don't need this.
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 check if the vertex connectivity is more than 3. What if the graph is only 2 or 1 connected ? what if the graph is not connected ?
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.
Whoops, I was getting my connectivity rules reversed. It should be fixed now. If the connectivity is more than three, that's actually okay; it's the reverse situation which induces a problem.
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.
a call to$O(n)$ . You should better first define a mapping from vertices to index in V like
V.index(u)
takes timevertex_to_int = {v: I for I, v in enumerate(V)}
and then use it. Each query will then be constant time.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.
Okay good catch fixed!