Opened 4 years ago
Closed 4 years ago
#24051 closed enhancement (fixed)
Issue with edge_colors when plotting undirected multi graph
Reported by:  dcoudert  Owned by:  

Priority:  major  Milestone:  sage8.1 
Component:  graph theory  Keywords:  
Cc:  slelievre  Merged in:  
Authors:  David Coudert  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  5fe4c3c (Commits, GitHub, GitLab)  Commit:  5fe4c3cf2bea4ee2647209e3ee787605db57432b 
Dependencies:  Stopgaps: 
Description
The following example has been reported here. The plot has 4 edges, 2 red and 2 black, while the multi graph has only 2 edges.
sage: G = Graph([(0,1), (0,1)], multiedges=True) sage: G.plot(edge_colors={"red":[(1,0)]})
Change History (10)
comment:1 Changed 4 years ago by
 Branch set to u/dcoudert/24051
 Cc slelievre added
 Commit set to e659503b489cff0c6496809c38dc038ee1adceb6
 Status changed from new to needs_review
comment:2 Changed 4 years ago by
I believe the problem comes from the fact that edges_drawn
is being built from the edge_colors
does not take into account when the input is out of order. Thus, the black edges get doubly added in the # Add unspecified edges
part. So I think more care is needed there in the undirected case. So I feel that this is what needs to be addressed.
comment:3 Changed 4 years ago by
 Commit changed from e659503b489cff0c6496809c38dc038ee1adceb6 to 4a3d686539b5f62a974fd62c857260354d7a35ee
Branch pushed to git repo; I updated commit sha1. New commits:
4a3d686  trac #24051: second trial

comment:4 Changed 4 years ago by
This is simpler.
comment:5 Changed 4 years ago by
Because of shortcircuiting, you can simply do:
if ((edge[0],edge[1],edge[2]) not in edges_drawn and ( self._graph.is_directed() or (edge[1],edge[0],edge[2]) not in edges_drawn) ):
comment:6 Changed 4 years ago by
 Commit changed from 4a3d686539b5f62a974fd62c857260354d7a35ee to 5fe4c3cf2bea4ee2647209e3ee787605db57432b
Branch pushed to git repo; I updated commit sha1. New commits:
5fe4c3c  trac #24051: simpler test

comment:7 Changed 4 years ago by
You are perfectly right.
comment:8 Changed 4 years ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
The plots I tested all look good. Thanks.
comment:9 Changed 4 years ago by
Thanks! You may want to look at #13827 too.
comment:10 Changed 4 years ago by
 Branch changed from u/dcoudert/24051 to 5fe4c3cf2bea4ee2647209e3ee787605db57432b
 Resolution set to fixed
 Status changed from positive_review to closed
This is certainly not the smartest fix, but it does the job. If someone has a smarter solution, feel free to change the branch.
New commits:
trac #24051: fix reported issue