Opened 5 years ago
Closed 5 years ago
#19390 closed enhancement (fixed)
Update the documentation of Graph/DiGraph constructors
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.10 
Component:  graph theory  Keywords:  
Cc:  vdelecroix, dcoudert, borassi, tscrim, SimonKing  Merged in:  
Authors:  Nathann Cohen  Reviewers:  David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  3b72424 (Commits, GitHub, GitLab)  Commit:  3b724240f9aba068d4495b4600c3f4a9190d5043 
Dependencies:  #19385  Stopgaps: 
Description
All this documentation was pretty outdated, and some formats were missing.
I also tried to make it easier to read/parse, by displaying at the beginning of the INPUT blocks the different formats (and not just by giving their names).
In particular, I do not think that it is very important to burden the users with a 'format' flag in most cases, when there is no ambiguity. However, when the input is a matrix and the behaviour depends on its properties, it is advertised.
A lot of documentation is also removed: some of it is replaced by links toward dedicated modules (i.e. 'how to access the graph catalog') or simply removed: the __init__
entry in th reference manual is no place to explain how to compute connected components.
There is still a lot of verbosity in this module, and in particular all the file formats *also* appear in the module's documentation (and are, of course, outdated). This will be for later.
One of the points of this branch is also to ease maintainability: when the same things are documented in different places at once, we forget to update all of it when it would be needed. Short, and to the point: that's a reference manual, not a tutorial.
Nathann
Change History (18)
comment:1 Changed 5 years ago by
 Branch set to u/ncohen/19390
 Commit set to 85bec44fea218b440c2bee16b3bd1f856b9afaca
 Status changed from new to needs_review
comment:2 followup: ↓ 4 Changed 5 years ago by
will be long to review...
in digraph.py
<http://en.wikipedia.org/wiki/Digraph_%28mathematics%29>`_.For a collection
one space missing``DiGraph({1: {2: 'a', 3:'b'} ,3:{2:'c'}})``  return a graph by
a digraph. Same in several placesDiGraph(a_nonsquare_matrix)
an incidence matrix ?edge `u,f` whenever ``f(u,v)`` is ``True``
> edgeu,v
``DiGraph(another_digraph)``  return a graph from a Sage graph,
from a Sage graph or digraph ?
in graph.py
edge `u,f` whenever ``f(u,v)`` is ``True``
> edgeu,v
``Graph(another_graph)``  return a graph from a Sage graph,
from a Sage graph or digraph ?only the integers 0, ..., n1, where n is the number of vertices.
math mode around 0, ..., n1 and n ?
may be more later ;)
comment:3 Changed 5 years ago by
 Commit changed from 85bec44fea218b440c2bee16b3bd1f856b9afaca to 4bc51d480bf228cf1bfbcaf25dc2737374a9c7e0
comment:4 in reply to: ↑ 2 ; followups: ↓ 5 ↓ 7 Changed 5 years ago by
Helloooooo,
in digraph.py
<http://en.wikipedia.org/wiki/Digraph_%28mathematics%29>`_.For a collection
one space missing
Done.
``DiGraph({1: {2: 'a', 3:'b'} ,3:{2:'c'}})``  return a graph by
a digraph. Same in several places
Sorry... Done.
DiGraph(a_nonsquare_matrix)
an incidence matrix ?
I did not change that, but I will change it if you want me to: let me just tell you why I wrote it this way. I chose to say "nonsquare_matrix" to hint that this is how a matrix is "automatically detected" to be an incidence matrix, and that to make sure one should use the explicit 'format' flag. That's why. If you think that it is still better the other way, tell me and I will update.
edge `u,f` whenever ``f(u,v)`` is ``True``
> edgeu,v
Done.
``DiGraph(another_digraph)``  return a graph from a Sage graph,
from a Sage graph or digraph ?
Done.
in graph.py
edge `u,f` whenever ``f(u,v)`` is ``True``
> edgeu,v
Done.
``Graph(another_graph)``  return a graph from a Sage graph,
from a Sage graph or digraph ?
Done.
only the integers 0, ..., n1, where n is the number of vertices.
math mode around 0, ..., n1 and n ?
Done, though I will do something about this parameter because it does not make any sense. It was apparently added during the igraph tickets, but its name (vertex_labels
) does not hint *at all* at igraph. The doc does not either. People could wonder about its effect with every other possible combinations of inputs, and it only appears in the code because of igraph. So I think we should remove it quick and deal with the igraph case otherwise.
Nathann
comment:5 in reply to: ↑ 4 ; followup: ↓ 6 Changed 5 years ago by
Hellooooooo!
Sorry if I am not very active in these tickets, but I'm quite busy in these days...
Just to say that when I inserted igraph
, the variable vertex_labels
was already there, and I only used it in my code. Indeed, you can find examples (written before igraph) where this variable was used (for instance, in cliques_get_max_clique_graph
).
Though, I have no idea what it was used for! The comment in the code before my modifications was:
 ``vertex_labels``  only for implementation == 'c_graph'. Whether to allow any object as a vertex (slower), or only the integers 0, ..., n1, where n is the number of vertices.
Hope this helps!
Michele
only the integers 0, ..., n1, where n is the number of vertices.
math mode around 0, ..., n1 and n ?Done, though I will do something about this parameter because it does not make any sense. It was apparently added during the igraph tickets, but its name (
vertex_labels
) does not hint *at all* at igraph. The doc does not either. People could wonder about its effect with every other possible combinations of inputs, and it only appears in the code because of igraph. So I think we should remove it quick and deal with the igraph case otherwise.Nathann
comment:6 in reply to: ↑ 5 Changed 5 years ago by
Yooooooo,
Sorry if I am not very active in these tickets, but I'm quite busy in these days...
Okay okay. I write these tickets because I want to have some Graph(filename)
constructor in order to not meet again this problem I had with your .nde files.
Just to say that when I inserted
igraph
, the variablevertex_labels
was already there, and I only used it in my code. Indeed, you can find examples (written before igraph) where this variable was used (for instance, incliques_get_max_clique_graph
).Though, I have no idea what it was used for!
In cliques_get_max_clique_graph
it seems to be an argument of .show()
(to display the labels) not an argument of Graph.__init__
. In the constructor, the only occurrence of this vertex_labels
argument is in the igraph code. If it is user anywhere else, well, it is actually ignored. It is not very bad anyway, just something that has to be done.
Nathann
comment:7 in reply to: ↑ 4 Changed 5 years ago by
DiGraph(a_nonsquare_matrix)
an incidence matrix ?I did not change that, but I will change it if you want me to: let me just tell you why I wrote it this way. I chose to say "nonsquare_matrix" to hint that this is how a matrix is "automatically detected" to be an incidence matrix, and that to make sure one should use the explicit 'format' flag. That's why. If you think that it is still better the other way, tell me and I will update.
Now I understand and I agree with you.
comment:8 Changed 5 years ago by
 Commit changed from 4bc51d480bf228cf1bfbcaf25dc2737374a9c7e0 to d64e45b7979eec1d32b42314adf3bfd33544ae08
Branch pushed to git repo; I updated commit sha1. New commits:
d64e45b  trac #19390: add a *symmetric* somewhere in the doc

comment:9 Changed 5 years ago by
Added a mention that a function must be symmetric, because of the following thread: https://groups.google.com/d/topic/sagedevel/TYpyrKCw8Zc/discussion
comment:10 Changed 5 years ago by
 Commit changed from d64e45b7979eec1d32b42314adf3bfd33544ae08 to e2dc0d49d824434da5b78a095936a8a0709ee150
Branch pushed to git repo; I updated commit sha1. New commits:
e2dc0d4  trac #19390: Merged with 6.10.beta0

comment:11 Changed 5 years ago by
 Commit changed from e2dc0d49d824434da5b78a095936a8a0709ee150 to e6223738b1c8f476333c8702c841e0f7c3f9a84f
Branch pushed to git repo; I updated commit sha1. New commits:
e622373  trac #19390: Illustration of the dangers of nonsymetric functions in the graph constructor

comment:12 Changed 5 years ago by
 Commit changed from e6223738b1c8f476333c8702c841e0f7c3f9a84f to 3b724240f9aba068d4495b4600c3f4a9190d5043
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3b72424  trac #19390: Illustration of the dangers of nonsymetric functions in the graph constructor

comment:13 Changed 5 years ago by
Hello,
the patch passes all tests and the produced doc is much better. For me the patch is good to go, but you will certainly have to rebase it on last beta first.
David.
comment:14 Changed 5 years ago by
 Reviewers set to David Coudert
comment:15 Changed 5 years ago by
Hello !
Thank you for the review ! It seems that there is nothing to rebase  there is no conflict with the latest beta.
Nathann
comment:16 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:17 Changed 5 years ago by
Good.
comment:18 Changed 5 years ago by
 Branch changed from u/ncohen/19390 to 3b724240f9aba068d4495b4600c3f4a9190d5043
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
trac #19381: adjacency matrix
trac #19381: Fix seidel adjacency
trac #19381: Incidence matrix
trac #19381: dict_of_dicts
trac #19381: dict of lists
trac #19381: dig6
trac #19381: Typo
trac #19385: Refactor DiGraph.__init__
trac #19385: Dictionary of lists for a digraph with labelled multiple edges. How I wish I could forget those stupid things.
trac #19390: Update the documentation of Graph/DiGraph constructors