-
-
Notifications
You must be signed in to change notification settings - Fork 465
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?
Conversation
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.
The coding style will have to be improved, but let's start with the most important.
if (len(external_face) < 3): | ||
raise ValueError("External face must have at least 3 vertices") | ||
|
||
G = Graph(self) |
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.
external_face_ordered = C.depth_first_search(start=external_face[0]) | ||
|
||
from sage.graphs.connectivity import vertex_connectivity | ||
if (vertex_connectivity(G, k=4)): |
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.
else: | ||
nv = G.neighbors(v) | ||
for u in nv: | ||
j = V.index(u) |
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 V.index(u)
takes time vertex_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!
…timized unnecessary call to .index()
…ge into tutte-embedding-graph-layout
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.
- Is the proposed method working on directed graphs ? if not, it should be in
graph.py
and not ingeneric_graph.py
- you should add some tests showing the behavior of the method when the connectivity requirement is not fulfilled, or when the graph is not planar
- For all comments, please use 80 columns mode (lines of length at most 80).
|
||
Compute graph layout based on a Tutte embedding. | ||
|
||
The graph must be 3-connected and planar. |
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
|
||
INPUT: | ||
|
||
- ``external_face`` -- list; the external face to be made a polygon |
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:
H = Graph(self) # take a (undirected) copy H of the graph
u, v = next(H.edge_iterator(labels=False) # take any edge (u, v) of H
H.delete_edge(u, v) # remove edge (u, v) from H
C = H.shortest_path(v, u) # Compute a shortest path from v to u in H minus (u, v)
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.
EXAMPLES:: | ||
|
||
sage: g = graphs.WheelGraph(n=7) | ||
sage: g.layout_tutte(external_face=[1, 2, 3, 4, 5, 6]) |
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...)
File "src/sage/graphs/generic_graph.py", line 21041, in sage.graphs.generic_graph.GenericGraph.layout_tutte
Failed example:
g.layout_tutte(external_face=[1, 2, 3, 4, 5, 6])
Expected:
{1: (-0.499999999999999, 0.866025403784438),
2: (0.500000000000000, 0.866025403784439),
3: (1.00000000000000, 8.85026869717109e-17),
4: (0.500000000000000, -0.866025403784439),
5: (-0.500000000000001, -0.866025403784438),
6: (-1.00000000000000, 4.55896726715916e-16),
0: (-5.55111512312578e-17, 1.86944233679563e-16)}
Got:
{0: (4.16333634234434e-16, 7.42055745992141e-16),
1: (-0.499999999999999, 0.866025403784439),
2: (0.500000000000000, 0.866025403784440),
3: (1.00000000000000, 7.31549942167514e-16),
4: (0.500000000000000, -0.866025403784438),
5: (-0.500000000000000, -0.866025403784437),
6: (-1.00000000000000, 1.29124025055007e-15)}
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.
The function is intended to work on digraphs as well. Since it's an embedding, it just embeds the digraph as if it were a simple graph. I've added more examples for when one of the requirements is not met. Can you be more specific about the 80 column mode? For what comments? These on GitHub or in the actual code? Sorry, this is my first time contributing. |
Adds enhancement requested in #38410.
For 3-connected and planar graphs (https://en.wikipedia.org/wiki/Polyhedral_graph), there exists an embedding into
the plane called the Tutte embedding. In the Tutte embedding, an outer face is set, and all other vertices are placed inside
the outer face such that they are at the mean position of their neighbors.
(https://en.wikipedia.org/wiki/Tutte_embedding)
📝 Checklist
⌛ Dependencies