Skip to content

Commit

Permalink
Trac #33759: storage of graph embeddings buggy
Browse files Browse the repository at this point in the history 
This ticket fixes the following issue
{{{
sage: G = Graph([(1,4), (2, 3)])
sage: G.is_planar(set_embedding=True, set_pos=True)
sage: G.delete_vertices([3])
sage: G.is_planar()
------------------------------------------------------------------------
---
KeyError                                  Traceback (most recent call
last)
Input In [27], in <cell line: 4>()
      2 G.is_planar(set_embedding=True)
      3 G.delete_vertices([Integer(3)])
----> 4 G.is_planar()

File /usr/lib/python3.10/site-
packages/sage/graphs/generic_graph.py:4995, in
GenericGraph.is_planar(self, on_embedding, kuratowski, set_embedding,
set_pos)
   4993 if hasattr(G, '_immutable'):
   4994     G = copy(G)
-> 4995 planar = is_planar(G,kuratowski=kuratowski, set_pos=set_pos,
set_embedding=set_embedding)
   4996 if kuratowski:
   4997     bool_result = planar[0]

File /usr/lib/python3.10/site-packages/sage/graphs/planarity.pyx:114, in
sage.graphs.planarity.is_planar
(build/cythonized/sage/graphs/planarity.c:2071)()
    112 cdef dict ffrom = {vvv: i + 1 for i, vvv in enumerate(listto)}
    113 cdef dict to = {i + 1: vvv for i, vvv in enumerate(listto)}
--> 114 g.relabel(ffrom)
    115
    116 cdef graphP theGraph

File /usr/lib/python3.10/site-
packages/sage/graphs/generic_graph.py:21878, in
GenericGraph.relabel(self, perm, inplace, return_map, check_input,
complete_partial_function, immutable)
  21876                 new_attr[perm[v]] = value
  21877             else:
> 21878                 new_attr[perm[v]] = [perm[w] for w in value]
  21880         setattr(self, attr, new_attr)
  21882 if return_map:

File /usr/lib/python3.10/site-
packages/sage/graphs/generic_graph.py:21878, in <listcomp>(.0)
  21876                 new_attr[perm[v]] = value
  21877             else:
> 21878                 new_attr[perm[v]] = [perm[w] for w in value]
  21880         setattr(self, attr, new_attr)
  21882 if return_map:

KeyError: 3
}}}

It also deprecates parameter `circular` in method `is_planar`.

----
Follow up: #33760, #33769

URL: https://trac.sagemath.org/33759
Reported by: vdelecroix
Ticket author(s): Vincent Delecroix
Reviewer(s): David Coudert
  • Loading branch information
Release Manager committed May 9, 2022
2 parents 5b8119a + f2b86c7 commit 7020df1
Show file tree
Hide file tree
Showing 2 changed files with 147 additions and 70 deletions.
154 changes: 118 additions & 36 deletions src/sage/graphs/generic_graph.py
View file
Expand Up @@ -5116,6 +5116,15 @@ def is_planar(self, on_embedding=None, kuratowski=False, set_embedding=False, se
sage: posets.BooleanLattice(3).cover_relations_graph().is_planar()
True

:trac:`33759`::

sage: G = Graph([(1, 2)])
sage: for set_embedding, set_pos in ((True,True), (True,False), (False, True), (False, False)):
....: G = Graph([(1, 2)])
....: assert G.is_planar(set_embedding=set_embedding, set_pos=set_pos)
....: assert (hasattr(G, '_embedding') and G._embedding is not None) == set_embedding, (set_embedding, set_pos)
....: assert (hasattr(G, '_pos') and G._pos is not None) == set_pos, (set_embedding, set_pos)

Corner cases::

sage: graphs.EmptyGraph().is_planar()
Expand Down Expand Up @@ -5239,13 +5248,13 @@ def is_circular_planar(self, on_embedding=None, kuratowski=False,
sage: g439.is_circular_planar(kuratowski=True, boundary=[1, 2, 3])
(True, None)
sage: g439.get_embedding()
{1: [7, 5],
2: [5, 6],
3: [6, 7],
4: [7, 6, 5],
5: [1, 4, 2],
6: [2, 4, 3],
7: [3, 4, 1]}
{1: [5, 7],
2: [6, 5],
3: [7, 6],
4: [5, 6, 7],
5: [2, 4, 1],
6: [3, 4, 2],
7: [1, 4, 3]}

Order matters::

Expand All @@ -5262,6 +5271,22 @@ def is_circular_planar(self, on_embedding=None, kuratowski=False,

TESTS:

Non-simple graphs::

sage: Graph([(1, 1)], loops=True).is_circular_planar(set_embedding=True)
Traceback (most recent call last):
...
NotImplementedError: cannot compute with embeddings of multiple-edged or looped graphs

Argument saving::

sage: G = Graph([(1, 2)])
sage: for set_embedding, set_pos in ((True,True), (True,False), (False, True), (False, False)):
....: G = Graph([(1, 2)])
....: assert G.is_circular_planar(set_embedding=set_embedding, set_pos=set_pos)
....: assert (hasattr(G, '_embedding') and G._embedding is not None) == set_embedding, (set_embedding, set_pos)
....: assert (hasattr(G, '_pos') and G._pos is not None) == set_pos, (set_embedding, set_pos)

Corner cases::

sage: graphs.EmptyGraph().is_circular_planar()
Expand All @@ -5279,6 +5304,12 @@ def is_circular_planar(self, on_embedding=None, kuratowski=False,
if self.order() > 3 and self.size() > 2 * self.order() - 3:
return False

if self.has_multiple_edges() or self.has_loops():
if set_embedding or (on_embedding is not None) or set_pos:
raise NotImplementedError("cannot compute with embeddings of multiple-edged or looped graphs")
else:
return self.to_simple().is_circular_planar(kuratowski=kuratowski, boundary=boundary, ordered=ordered)

if boundary is None:
boundary = self

Expand All @@ -5287,7 +5318,7 @@ def is_circular_planar(self, on_embedding=None, kuratowski=False,
from sage.graphs.planarity import is_planar
graph = Graph(self)
if hasattr(graph, '_embedding'):
del(graph._embedding)
del graph._embedding

# Adds a new vertex to the graph and connects it to all vertices of the
# boundary
Expand All @@ -5308,32 +5339,29 @@ def is_circular_planar(self, on_embedding=None, kuratowski=False,

graph.add_edges(extra_edges)

result = is_planar(graph, kuratowski=kuratowski, set_embedding=set_embedding, circular=True)
result = is_planar(graph, kuratowski=kuratowski, set_embedding=set_embedding, set_pos=set_pos)

if kuratowski:
bool_result = result[0]
else:
bool_result = result

if bool_result:
graph.delete_vertex(extra)
graph.delete_edges(extra_edges)

if hasattr(graph,'_embedding'):
if set_embedding:
# strip the embedding to fit original graph
for u,v in extra_edges:
graph._embedding[u].pop(graph._embedding[u].index(v))
graph._embedding[v].pop(graph._embedding[v].index(u))
for w in boundary:
graph._embedding[w].pop(graph._embedding[w].index(extra))

if set_embedding:
self._embedding = graph._embedding.copy()
del graph._embedding[extra]
for u, v in extra_edges:
graph._embedding[u].remove(v)
graph._embedding[v].remove(u)
for v in boundary:
graph._embedding[v].remove(extra)
self._embedding = graph._embedding

if set_pos:
# strip the position
del graph._pos[extra]
self._pos = graph._pos

if (set_pos and set_embedding):
self.layout(layout="planar", save_pos=True, test=False)

del graph
return result

def layout_planar(self, set_embedding=False, on_embedding=None,
Expand Down Expand Up @@ -5538,7 +5566,7 @@ def layout_planar(self, set_embedding=False, on_embedding=None,
embedding_copy = {v: neighbors[:] for v, neighbors in G._embedding.items()}
else:
if on_embedding is not None:
G._check_embedding_validity(on_embedding,boolean=False)
G._check_embedding_validity(on_embedding, boolean=False)
if not G.is_planar(on_embedding=on_embedding):
raise ValueError('provided embedding is not a planar embedding for %s'%self )
G.set_embedding(on_embedding)
Expand Down Expand Up @@ -10257,18 +10285,39 @@ def delete_vertex(self, vertex, in_order=False):
{0: 'no delete', 2: None, 3: None, 4: None}
sage: G.get_pos()
{0: (0, 0), 2: (2, 0), 3: (3, 0), 4: (4, 0)}

TESTS:

Test that :trac:`33759` is fixed::

sage: G = Graph([(1, 4), (2, 3)])
sage: G.is_planar(set_embedding=True)
True
sage: G.delete_vertex(3)
sage: G.is_planar()
True
"""
if in_order:
vertex = self.vertices()[vertex]
if vertex not in self:
raise ValueError("vertex (%s) not in the graph"%str(vertex))

self._backend.del_vertex(vertex)
attributes_to_update = ('_pos', '_assoc', '_embedding')
attributes_to_update = ('_pos', '_assoc')
for attr in attributes_to_update:
if hasattr(self, attr) and getattr(self, attr) is not None:
getattr(self, attr).pop(vertex, None)

if hasattr(self, '_embedding'):
embedding = self._embedding
if embedding is not None:
neighbors = set(self.neighbor_iterator(vertex))
neighbors.discard(vertex)
for w in neighbors:
embedding[w] = [x for x in embedding[w] if x != vertex]
embedding.pop(vertex, None)

self._backend.del_vertex(vertex)

def delete_vertices(self, vertices):
"""
Delete vertices from the (di)graph taken from an iterable container of
Expand All @@ -10290,19 +10339,39 @@ def delete_vertices(self, vertices):
...
ValueError: vertex (1) not in the graph

TESTS:

Test that :trac:`33759` is fixed::

sage: G = Graph([(1, 4), (2, 3)])
sage: G.is_planar(set_embedding=True)
True
sage: G.delete_vertices([3])
sage: G.is_planar()
True
"""
vertices = list(vertices)
for vertex in vertices:
if vertex not in self:
raise ValueError("vertex (%s) not in the graph"%str(vertex))
for v in vertices:
if v not in self:
raise ValueError("vertex (%s) not in the graph"%str(v))

self._backend.del_vertices(vertices)
attributes_to_update = ('_pos', '_assoc', '_embedding')
for attr in attributes_to_update:
for attr in ('_pos', '_assoc'):
if hasattr(self, attr) and getattr(self, attr) is not None:
attr_dict = getattr(self, attr)
for vertex in vertices:
attr_dict.pop(vertex, None)
for v in vertices:
attr_dict.pop(v, None)

if hasattr(self, '_embedding'):
embedding = self._embedding
if embedding is not None:
neighbors = set().union(*[self.neighbor_iterator(v) for v in vertices])
neighbors.difference_update(vertices)
for w in neighbors:
embedding[w] = [x for x in embedding[w] if x not in vertices]
for v in vertices:
embedding.pop(v, None)

self._backend.del_vertices(vertices)

def has_vertex(self, vertex):
"""
Expand Down Expand Up @@ -11429,6 +11498,7 @@ def delete_edge(self, u, v=None, label=None):
except Exception:
u, v = u
label = None

self._backend.del_edge(u, v, label, self._directed)

def delete_edges(self, edges):
Expand Down Expand Up @@ -19024,6 +19094,11 @@ def layout(self, layout=None, pos=None, dim=2, save_pos=False, **options):
('1', 1): [2.26..., -0.87...]}

sage: g.layout(layout="acyclic_dummy", save_pos=True)
{('0', 0): [0.3..., 0],
('0', 1): [0.3..., 1],
('1', 0): [0.6..., 0],
('1', 1): [0.6..., 1]}
sage: g.get_pos()
{('0', 0): [0.3..., 0],
('0', 1): [0.3..., 1],
('1', 0): [0.6..., 0],
Expand Down Expand Up @@ -19085,6 +19160,13 @@ def layout(self, layout=None, pos=None, dim=2, save_pos=False, **options):
use this feature for all the predefined graphs classes (like for the
Petersen graph, ...), rather than systematically building the layout
at construction time.

TESTS::

sage: for style in ('spring', 'planar', 'circular', 'forest'):
....: for G in [Graph([(1, 2)]), Graph([(1, 2), (2, 3), (3, 4)])]:
....: pos = G.layout(style, save_pos=True)
....: assert G._pos is not None
"""
if layout is None:
if pos is None:
Expand Down
63 changes: 29 additions & 34 deletions src/sage/graphs/planarity.pyx
View file
Expand Up @@ -29,7 +29,7 @@ cdef extern from "planarity/graph.h":
cdef int gp_Embed(graphP theGraph, int embedFlags)
cdef int gp_SortVertices(graphP theGraph)

def is_planar(g, kuratowski=False, set_pos=False, set_embedding=False, circular=False):
def is_planar(g, kuratowski=False, set_pos=False, set_embedding=False, circular=None):
r"""
Check whether ``g`` is planar using Boyer's planarity algorithm.
Expand All @@ -54,8 +54,7 @@ def is_planar(g, kuratowski=False, set_pos=False, set_embedding=False, circular=
combinatorial embedding returned (see
:meth:`~sage.graphs.generic_graph.GenericGraph.get_embedding`)
- ``circular`` -- boolean (default: ``False``); whether to test for circular
planarity
- ``circular`` -- deprecated argument
EXAMPLES::
Expand Down Expand Up @@ -86,7 +85,21 @@ def is_planar(g, kuratowski=False, set_pos=False, set_embedding=False, circular=
....: if (not g.is_connected() or i == 0):
....: continue
....: _ = g.is_planar(set_embedding=True, set_pos=True)
Argument saving::
sage: G = Graph([(1, 2)])
sage: for set_embedding, set_pos in ((True,True), (True,False), (False, True), (False, False)):
....: G = Graph([(1, 2)])
....: assert is_planar(G, set_embedding=set_embedding, set_pos=set_pos)
....: assert (hasattr(G, '_embedding') and G._embedding is not None) == set_embedding, (set_embedding, set_pos)
....: assert (hasattr(G, '_pos') and G._pos is not None) == set_pos, (set_embedding, set_pos)
"""
if circular is not None:
from sage.misc.superseded import deprecation
deprecation(33759, 'the circular argument of is_planar is deprecated and has no effect')

if set_pos and not g.is_connected():
raise ValueError("is_planar() cannot set vertex positions for a disconnected graph")

Expand Down Expand Up @@ -158,41 +171,23 @@ def is_planar(g, kuratowski=False, set_pos=False, set_embedding=False, circular=
else:
return False
else:
if not circular:
if set_pos or set_embedding:
emb_dict = {}
#for i in range(theGraph.N):
for i in range(1, theGraph.N + 1):
linked_list = []
j = theGraph.V[i].link[1]
while j:
linked_list.append(to[theGraph.E[j].neighbor])
j = theGraph.E[j].link[1]
emb_dict[to[i]] = linked_list
if set_embedding:
emb_dict = {}
#for i in range(theGraph.N):
for i in range(1, theGraph.N + 1):
linked_list = []
j = theGraph.V[i].link[1]
while j:
linked_list.append(to[theGraph.E[j].neighbor])
j = theGraph.E[j].link[1]
emb_dict[to[i]] = linked_list
g._embedding = emb_dict
if set_pos:
g.layout(layout='planar', save_pos=True)
else:
if set_embedding:
# Take counter-clockwise embedding if circular planar test
# Also, pos must be set after removing extra vertex and edges

# This is separated out here for now because in the circular case,
# setting positions would have to come into play while the extra
# "wheel" or "star" is still part of the graph.

emb_dict = {}
#for i in range(theGraph.N):
for i in range(1, theGraph.N + 1):
linked_list = []
j = theGraph.V[i].link[0]
while j:
linked_list.append(to[theGraph.E[j].neighbor])
j = theGraph.E[j].link[0]
emb_dict[to[i]] = linked_list
g._embedding = emb_dict
g.layout(layout='planar', save_pos=True, on_embedding=emb_dict)

gp_Free(&theGraph)
if kuratowski:
return (True,None)
return (True, None)
else:
return True

0 comments on commit 7020df1

Please sign in to comment.