Opened 4 years ago

Closed 4 years ago

#19390 closed enhancement (fixed)

Update the documentation of Graph/DiGraph constructors

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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) 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 4 years ago by ncohen

  • Branch set to u/ncohen/19390
  • Commit set to 85bec44fea218b440c2bee16b3bd1f856b9afaca
  • Status changed from new to needs_review

Last 10 new commits:

f0f8ee4trac #19381: adjacency matrix
7a51fdetrac #19381: Fix seidel adjacency
5abbd4btrac #19381: Incidence matrix
cf7f515trac #19381: dict_of_dicts
f9fc7a8trac #19381: dict of lists
dd28d93trac #19381: dig6
9bdf527trac #19381: Typo
cf53613trac #19385: Refactor DiGraph.__init__
99c8601trac #19385: Dictionary of lists for a digraph with labelled multiple edges. How I wish I could forget those stupid things.
85bec44trac #19390: Update the documentation of Graph/DiGraph constructors

comment:2 follow-up: Changed 4 years ago by dcoudert

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 places
  • DiGraph(a_nonsquare_matrix) an incidence matrix ?
  • edge `u,f` whenever ``f(u,v)`` is ``True`` -> edge u,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`` -> edge u,v
  • ``Graph(another_graph)`` -- return a graph from a Sage graph, from a Sage graph or digraph ?
  • only the integers 0, ..., n-1, where n is the number of vertices. math mode around 0, ..., n-1 and n ?

may be more later ;)

comment:3 Changed 4 years ago by git

  • Commit changed from 85bec44fea218b440c2bee16b3bd1f856b9afaca to 4bc51d480bf228cf1bfbcaf25dc2737374a9c7e0

Branch pushed to git repo; I updated commit sha1. New commits:

27cfe2ctrac #19390: Merged with 6.9
4bc51d4trac #19390: Typos

comment:4 in reply to: ↑ 2 ; follow-ups: Changed 4 years ago by ncohen

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`` -> edge u,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`` -> edge u,v

Done.

  • ``Graph(another_graph)`` -- return a graph from a Sage graph, from a Sage graph or digraph ?

Done.

  • only the integers 0, ..., n-1, where n is the number of vertices. math mode around 0, ..., n-1 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 ; follow-up: Changed 4 years ago by borassi

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, ..., n-1, where n is the number of vertices.

Hope this helps!

Michele

  • only the integers 0, ..., n-1, where n is the number of vertices. math mode around 0, ..., n-1 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 4 years ago by ncohen

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 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!

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 4 years ago by dcoudert

  • 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 4 years ago by git

  • Commit changed from 4bc51d480bf228cf1bfbcaf25dc2737374a9c7e0 to d64e45b7979eec1d32b42314adf3bfd33544ae08

Branch pushed to git repo; I updated commit sha1. New commits:

d64e45btrac #19390: add a *symmetric* somewhere in the doc

comment:9 Changed 4 years ago by ncohen

Added a mention that a function must be symmetric, because of the following thread: https://groups.google.com/d/topic/sage-devel/TYpyrKCw8Zc/discussion

comment:10 Changed 4 years ago by git

  • Commit changed from d64e45b7979eec1d32b42314adf3bfd33544ae08 to e2dc0d49d824434da5b78a095936a8a0709ee150

Branch pushed to git repo; I updated commit sha1. New commits:

e2dc0d4trac #19390: Merged with 6.10.beta0

comment:11 Changed 4 years ago by git

  • Commit changed from e2dc0d49d824434da5b78a095936a8a0709ee150 to e6223738b1c8f476333c8702c841e0f7c3f9a84f

Branch pushed to git repo; I updated commit sha1. New commits:

e622373trac #19390: Illustration of the dangers of nonsymetric functions in the graph constructor

comment:12 Changed 4 years ago by git

  • Commit changed from e6223738b1c8f476333c8702c841e0f7c3f9a84f to 3b724240f9aba068d4495b4600c3f4a9190d5043

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

3b72424trac #19390: Illustration of the dangers of nonsymetric functions in the graph constructor

comment:13 Changed 4 years ago by dcoudert

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 4 years ago by dcoudert

  • Reviewers set to David Coudert

comment:15 Changed 4 years ago by ncohen

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 4 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:17 Changed 4 years ago by dcoudert

Good.

comment:18 Changed 4 years ago by vbraun

  • Branch changed from u/ncohen/19390 to 3b724240f9aba068d4495b4600c3f4a9190d5043
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.