Opened 3 years ago

Closed 3 years ago

#12816 closed enhancement (fixed)

Documentation and list of Graph functions

Reported by: ncohen Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.1
Component: graph theory Keywords:
Cc: dcoudert Merged in: sage-5.1.beta1
Authors: Nathann Cohen Reviewers: David Coudert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #12743, #12903 Stopgaps:

Description (last modified by ncohen)

This patch only touches documentation. It mainly adds lists of function to the top of important Graph files so that we can, at long long long last, see what on earth those objects can do. This list will have to be maintained manually, so this is a bad solution to a problem that should be fixed by Sphinx. Only it is not, and we really need to have such lists !

Apply:

Nathann

Attachments (1)

trac_12816-v2.patch (96.6 KB) - added by ncohen 3 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 3 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by dcoudert

  • Status changed from needs_review to needs_info

The patch passes successfully a sage -t --long -force_lib --verbose sage/graphs test, and the doc builds properly. The result is really nice.

Clearly, it would be nice to have such sections on top of all pages.

For graph colorings, you have organized the methods by type. What about organizing methods by type in graph.py, generic_graph.py, etc. ?

I like this patch.

comment:3 Changed 3 years ago by dcoudert

  • Reviewers set to David Coudert

comment:4 Changed 3 years ago by ncohen

Done here ! But I would really like to get a good answer from the Sphinx guys, for the html file produced really is awful :-p

https://groups.google.com/d/topic/sphinx-dev/GWLtoAHGOo4/discussion

Nathann

comment:5 Changed 3 years ago by dcoudert

In the produced html files, you can see that:

  • The width of columns is proportional to the number of '=' per column
  • The width of columns of table in same file is consistent: 45-55, 54-46, ...
  • However, the width of the table is never indicated. If you add for instance in file graph.html a "width=100%" option to each table directives, the rendering is much better (columns in different tables have same width.

Unfortunately, I don't think sphinx allows for adding such directive :(

This is exactly the same story with the other type of tables (+----------+---------+).

comment:6 Changed 3 years ago by dcoudert

A better solution is to use csv-table (http://docutils.sourceforge.net/docs/ref/rst/directives.html#table). Bellow is an example for graph.py

**Graph basic operations:** 
 
.. csv-table:: 
    :class: contentstable 
    :header: "Methods", "Description" 
    :widths: 50, 50 
    :delim: | 
 
    :meth:`~Graph.write_to_eps` | Writes a plot of the graph to ``filename`` in ``eps`` format. 
    :meth:`~Graph.to_undirected` | Since the graph is already undirected, simply returns a copy of itself. 
    :meth:`~Graph.to_directed` | Returns a directed version of the graph. 
    :meth:`~Graph.sparse6_string` | Returns the sparse6 representation of the graph as an ASCII string. 
    :meth:`~Graph.graph6_string` | Returns the graph6 representation of the graph as an ASCII string. 
    :meth:`~Graph.bipartite_sets` | Returns `(X,Y)` where X and Y are the nodes in each bipartite set of graph. 
    :meth:`~Graph.bipartite_color` | Returns a dictionary with vertices as the keys and the color class as the values. 
    :meth:`~Graph.is_directed` | Since graph is undirected, returns False. 
 
**Distances:** 
 
.. csv-table:: 
    :class: contentstable 
    :header: "Methods", "Description" 
    :widths: 50, 50 
    :delim: | 
 
    :meth:`~Graph.centrality_closeness` | Returns the closeness centrality (1/average distance to all vertices) 
    :meth:`~Graph.centrality_degree` | Returns the degree centrality 
    :meth:`~Graph.centrality_betweenness` | Returns the betweenness centrality 
 
**Graph properties:** 
 
.. csv-table:: 
    :class: contentstable 
    :header: "Methods", "Description" 
    :widths: 50, 50 
    :delim: | 
 
    :meth:`~Graph.is_prime` | Tests whether the current graph is prime. 
    :meth:`~Graph.is_split` | Returns ``True`` if the graph is a Split graph, ``False`` otherwise. 
    :meth:`~Graph.is_triangle_free` | Returns whether ``self`` is triangle-free 
    :meth:`~Graph.is_bipartite` | Returns True if graph G is bipartite, False if not. 
    :meth:`~Graph.is_line_graph` | Tests wether the graph is a line graph. 
    :meth:`~Graph.is_odd_hole_free` | Tests whether ``self`` contains an induced odd hole. 
    :meth:`~Graph.is_even_hole_free` | Tests whether ``self`` contains an induced even hole. 

The output is better, but we have another issue to solve.

In the original graph.html file produced when building the documentation, we have:

  • <table border="1" class="docutils">

Using the :class: contentstable directive, I obtain:

  • <table border="1" class="contentstable docutils">

But then the produced table is of width 90%.
Also, the ultimate solution would be to add in file doc/output/html/en/reference/_static/basic.css
an entry like

table.fullwidthtable {
    width: 100%;
}

and then to use the directive :class: fullwidthtable (or table100). Other options like center can also be added.

However, I don't know how to add such stuff into the basic.css file...

I think this is the best way to do it. Furthermore, it is much easier to use/maintain than the simple table solution.

D.

Last edited 3 years ago by dcoudert (previous) (diff)

comment:7 Changed 3 years ago by ncohen

  • Description modified (diff)
  • Status changed from needs_info to needs_review

Patch updated ! Honestly, I am pretty happy with this result. Thank you for this contentstable trick !!

I dedicate this patch to Emacs' macro feature, without which none of this would have been possible.

God I love those macros :-D

Nathann

comment:8 follow-up: Changed 3 years ago by dcoudert

Escape-Meta-Alt-Control-Shift is great ;-)

comment:9 in reply to: ↑ 8 Changed 3 years ago by ncohen

Escape-Meta-Alt-Control-Shift is great ;-)

It has become an illness, actually. My shortcut to shut my computer down is "CTRL + ALT + SHIFT + HOME" since last week. For the moment it looks likle this combination does not interfere with any emacs shortcut, which I find suspicious :-D

Nathann

comment:10 Changed 3 years ago by dcoudert

Very nice patch. Installation, compilation and docbuild OK on sage-5.0.beta13 !

I have 2 observations:

  • The width of the column should rather be 30, 70 instead of 20, 80. Of course it cannot be perfect, but depending on the size of the window of your navigator, the rendering is very different. Of course with a full screen window its perfect, but if the default size is not fullscreen, then column are not aligned between tables. It's not a big issue, but...
  • Small typo in file digraph.py: Acyclicityy -> acyclicity

This should definitely be done in all files (not necessarily in this patch). For instance, I will modify patch #12716 to have the same kind of tables.

comment:11 Changed 3 years ago by ncohen

Dooooooooone !

Nathann

comment:12 Changed 3 years ago by dcoudert

This is much nicer ;-)

I did sage -t --long --force_lib sage/graphs/ and it fails (only) on generic_graph.py: TAB character found.
I think we have 2 TABs in the file (functions antisymmetric and is_clique).

The patch will then be good to go.

comment:13 Changed 3 years ago by dcoudert

  • Dependencies changed from 12743 to #12743

comment:14 Changed 3 years ago by ncohen

Pom Pom Pom....

Updated ! :-)

Nathann

comment:15 Changed 3 years ago by dcoudert

  • Status changed from needs_review to positive_review

All tests pass and the tables are really useful. Nice work Nathann !

comment:16 Changed 3 years ago by ncohen

Wouhouuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu !!!!

Or rather, in english :

WoooooooooooHooooooooooooooooooooooo? !! :-D

Nathann

comment:17 Changed 3 years ago by jdemeyer

  • Authors set to Nathann Cohen

comment:18 Changed 3 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Please keep

AUTHORS:

as before, or convince people on sage-devel that

**Authors**

is better.

Also, I'm not so fond of simply removing all whitespace lines in places where there are no other changes, as this can cause merge conflicts for future patches for no good reason.

comment:19 Changed 3 years ago by ncohen

  • Status changed from needs_work to positive_review

Hellooooooo Jeroen !

I just updated the patch so that the occurrences of * * Authors * * disappear. I think that it is better to write it in bold in the documentation of modules when there is actually a lot of doc as it is otherwise totally buried by the other informations (and it does not deserve to be granted a whole section).

Well, and I took care of only removing "all the trailing whitespaces" from files which I know are almost never updated. If I were to do that with graph.py or digraph.py the sage-combinat guys would kill me instantly :-D

Nathann

comment:20 Changed 3 years ago by jdemeyer

  • Milestone changed from sage-5.0 to sage-5.1

comment:21 Changed 3 years ago by ncohen

(rebased on beta14)

comment:22 Changed 3 years ago by dcoudert

all tests pass ;-)

comment:23 Changed 3 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This doesn't apply properly on sage-5.0.beta14 + #12743:

applying trac_12816-v2.patch
patching file sage/graphs/graph.py
Hunk #2 FAILED at 176
1 out of 4 hunks FAILED -- saving rejects to file sage/graphs/graph.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh trac_12816-v2.patch

comment:24 Changed 3 years ago by ncohen

  • Status changed from needs_work to positive_review

Just a trailing whitespace... Updated ! :-)

Nathann

comment:25 Changed 3 years ago by jdemeyer

  • Dependencies changed from #12743 to #12743, #12903
  • Status changed from positive_review to needs_work

Sorry, but this also conflicts with #12903. Could you rebase it on top of #12743 + #12903.

comment:26 Changed 3 years ago by ncohen

  • Status changed from needs_work to positive_review

No prob ! :-)

Nathann

Changed 3 years ago by ncohen

comment:27 Changed 3 years ago by jdemeyer

  • Merged in set to sage-5.1.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.