Opened 6 years ago
Closed 6 years ago
#21048 closed enhancement (fixed)
Graph plotting: vertex colors
Reported by:  Jori Mäntysalo  Owned by:  

Priority:  major  Milestone:  sage7.3 
Component:  graph theory  Keywords:  
Cc:  Paul Masson, krcisman, Travis Scrimshaw  Merged in:  
Authors:  Jori Mäntysalo  Reviewers:  Paul Masson 
Report Upstream:  N/A  Work issues:  
Branch:  34be2bb (Commits, GitHub, GitLab)  Commit:  34be2bb7941f2e6f343b9c88ef42e02d7ba77522 
Dependencies:  Stopgaps: 
Description
This is a first part of #13827.
Currently setting a color for one vertex will change default pink to blue in other vertices. This patch will correct it. Also we make it possible to set a default color with vertex_color
and have some vertices colored differently with vertex_colors
. To test try for example
sage: g=graphs.PetersenGraph() sage: g.show() sage: g.show(vertex_color='green') sage: g.show(vertex_colors={'red': [0,1], 'blue': [3,4]}) sage: g.show(vertex_color='green', vertex_colors={'red': [0,1], 'blue': [3,4]}) sage: g.show(vertex_colors='green')
Change History (19)
comment:1 Changed 6 years ago by
Branch:  → u/jmantysalo/vertexcolor 

comment:2 Changed 6 years ago by
Cc:  Paul Masson added 

Commit:  → b8e8b5c5a12cc94128a230bc2346a6f32d49ddb0 
Status:  new → needs_review 
comment:3 followup: 4 Changed 6 years ago by
Reviewers:  → Paul Masson 

Doctests pass. Documentation builds. Example runs as expected.
It looks like you took out some default rainbow coloring in the code. Since this is a part of Sage I'm unlikely to use much, I would like to understand the impact this will have on other users. While it's certainly better to have more control over the coloring, how many people are likely to complain if the rainbow coloring is gone?
comment:4 Changed 6 years ago by
Replying to paulmasson:
It looks like you took out some default rainbow coloring in the code.
I was not able to run that part of code. It seems to refer partition
option. However, currently
g = graphs.PetersenGraph() g.show(partition=[[1,2],[3,4]])
have same problem of changing default color for nonlisted vertices. With this patch they will stay pink, and also you can say
g.show(partition=[[1,2],[3,4]], vertex_color='gray')
comment:5 Changed 6 years ago by
Sidenote, there was also a plain error in documentation: "If a vertex is not listed, it looks invisible on the resulting plot (it does not get drawn)." was not true, and is not after this patch.
comment:6 followup: 7 Changed 6 years ago by
Cc:  Nathann Cohen krcisman Travis Scrimshaw added 

The current code doesn't appear to have a default color. Nonlisted vertices are given a random color, not a default color. This ticket modifies that behavior.
If that is a good idea then I'm fine with it, but I would rather have some additional input. Nathann Cohen wrote the original coloring code in 2009, so I'd like to hear from him or interested others on this change.
comment:7 Changed 6 years ago by
Replying to paulmasson:
The current code doesn't appear to have a default color. Nonlisted vertices are given a random color, not a default color.
Kind of, but always same color. Setting a color of one vertex to blue or green make others red, setting a color to red make others light blue etc.
If that is a good idea then I'm fine with it, but I would rather have some additional input. Nathann Cohen wrote the original coloring code in 2009, so I'd like to hear from him or interested others on this change.
Me too, but Nathann faded from developers some time ago. :=(
See for example https://groups.google.com/forum/#!topic/sagedevel/Z5kXJv6ZQA
comment:8 Changed 6 years ago by
Cc:  Nathann Cohen removed 

Jori, I started reading sagedevel some time after that thread, so thanks for sharing. I've seen some other comments since then about the same topic but didn't realize it was such an issue. I've removed him from this ticket.
I'd like to wait a couple days to see if KarlDieter or Travis want to chime in. If we don't hear from them soon then I'll set positive review. I certainly don't want to deter an active developer!
comment:9 Changed 6 years ago by
I don't have a real preference for this because I never really use it. If you want additional input, you're probably best posting to sagedevel.
comment:10 Changed 6 years ago by
Status:  needs_review → positive_review 

comment:11 Changed 6 years ago by
Status:  positive_review → needs_work 

sage t long src/sage/combinat/words/finite_word.py ********************************************************************** File "src/sage/combinat/words/finite_word.py", line 116, in sage.combinat.words.finite_word Failed example: st.show(word_labels=True) Expected nothing Got: doctest:270: DeprecationWarning: Use of vertex_colors=<string> is deprecated, use vertex_color=<string> and/or vertex_colors=<dict>. See http://trac.sagemath.org/21048 for details. ********************************************************************** 1 item had failures: 1 of 56 in sage.combinat.words.finite_word [1244 tests, 1 failure, 11.63 s] sage t long src/sage/combinat/words/suffix_trees.py ********************************************************************** File "src/sage/combinat/words/suffix_trees.py", line 867, in sage.combinat.words.suffix_trees.ImplicitSuffixTree.? Failed example: ImplicitSuffixTree(Word('cacao')).plot(word_labels=True) Expected: Graphics object consisting of 23 graphics primitives Got: doctest:270: DeprecationWarning: Use of vertex_colors=<string> is deprecated, use vertex_color=<string> and/or vertex_colors=<dict>. See http://trac.sagemath.org/21048 for details. Graphics object consisting of 23 graphics primitives ********************************************************************** 1 item had failures: 1 of 7 in sage.combinat.words.suffix_trees.ImplicitSuffixTree.? [245 tests, 1 failure, 3.03 s]
comment:12 Changed 6 years ago by
Commit:  b8e8b5c5a12cc94128a230bc2346a6f32d49ddb0 → 60b487d3101146d4b8b10f0ae5eec193613880cc 

Branch pushed to git repo; I updated commit sha1. New commits:
60b487d  Corrections to words.

comment:13 Changed 6 years ago by
Status:  needs_work → needs_review 

Badly corrected, but at least works. I guess words and posetsdirectories should be checked when graph plotting is ready.
comment:14 Changed 6 years ago by
vertex_colors
is defined as a dicitionary in suffix_trees.py
as it should be, so the changes you made aren't the best. The problem is the misspelling veretex_colors
on line 885. Please revert the changes you just made and correct that spelling. I've already tested this and it solves both doctest failures.
comment:15 Changed 6 years ago by
Commit:  60b487d3101146d4b8b10f0ae5eec193613880cc → 2e599755176bcdd903f9c2fee015df7e32345b25 

Branch pushed to git repo; I updated commit sha1. New commits:
2e59975  Revert "Corrections to words."

comment:16 Changed 6 years ago by
Commit:  2e599755176bcdd903f9c2fee015df7e32345b25 → 34be2bb7941f2e6f343b9c88ef42e02d7ba77522 

Branch pushed to git repo; I updated commit sha1. New commits:
34be2bb  Typo on variable name corrected.

comment:18 Changed 6 years ago by
Status:  needs_review → positive_review 

All doctests pass in combinat/words
and combinat/posets
comment:19 Changed 6 years ago by
Branch:  u/jmantysalo/vertexcolor → 34be2bb7941f2e6f343b9c88ef42e02d7ba77522 

Resolution:  → fixed 
Status:  positive_review → closed 
On a way to more customizable graph plotting.
New commits:
Modified setting of graph vertices colors.