Opened 9 years ago

Closed 9 years ago

#8513 closed defect (fixed)

Including documentation in the reference manual for some files related to graph theory

Reported by: abmasse Owned by: rlm
Priority: minor Milestone: sage-4.4
Component: graph theory Keywords: documentation, graph theory
Cc: ncohen, rlm Merged in: sage-4.4.alpha0
Authors: Alexandre Blondin Massé, Nathann Cohen Reviewers: Minh Van Nguyen, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

I noticed a few weeks ago while reviewing a patch that important files in the graph theory folder were not appearing anywhere in the reference manual. For instance, functions such as vertex_cut or edge_cut do not appear, as well as all functions defined only for directed graphs.

At that time, I thought about adding the missing files with the patch, but since there were a lot of warnings displayed by Sphinx while generating the documentation, I changed my mind.

I think it would be a good idea to use this ticket to fix this, but since it touches many files of graph theory, it may be hard to do it in a clean way. Someone has an idea of what would be the best approach ?

Attachments (3)

trac_8513_graph_theory_documentation-abm.patch (8.4 KB) - added by abmasse 9 years ago.
Adds digraph.py and generic_graph.py in the doctree
trac_8513_graph_theory_documentation-smallfixes.patch (13.0 KB) - added by mvngu 9 years ago.
trac_8513-reviewer.patch (1.4 KB) - added by mvngu 9 years ago.
reviewer patch

Download all attachments as: .zip

Change History (22)

comment:1 Changed 9 years ago by mvngu

Wait until Sage 4.3.4 is released. I expect that release to fix all the warnings relating to building the HTML and PDF versions of the Sage standard documentation. At least, I expect that release to fix such warnings for the reference manual.

comment:2 Changed 9 years ago by abmasse

  • Cc rlm added
  • Status changed from new to needs_work

Here are the warnings I get on my computer:

[~/Applications/sage/devel/sage-combinat/doc/en/reference]$ sage -docbuild reference html
sphinx-build -b html -d /Users/alexandre/Applications/sage/devel/sage/doc/output/doctrees/en/reference    /Users/alexandre/Applications/sage/devel/sage/doc/en/reference /Users/alexandre/Applications/sage/devel/sage/doc/output/html/en/reference
Running Sphinx v0.6.3
loading pickled environment... done
building [html]: targets for 612 source files that are out of date
updating environment: 2 added, 612 changed, 0 removed
reading sources... [100%] sage/symbolic/ring                   curve_morphismtorcfield
/Users/alexandre/Applications/sage/local/lib/python2.6/site-packages/sage/graphs/generic_graph.py:docstring of sage.graphs.generic_graph:3: (ERROR/3) Unexpected indentation.
/Users/alexandre/Applications/sage/local/lib/python2.6/site-packages/sage/graphs/generic_graph.py:docstring of sage.graphs.generic_graph.GenericGraph.minimum_outdegree_orientation:7: (WARNING/2) Block quote ends without a blank line; unexpected unindent.
/Users/alexandre/Applications/sage/local/lib/python2.6/site-packages/sage/graphs/generic_graph.py:docstring of sage.graphs.generic_graph.GenericGraph.spanning_trees_count:18: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
/Users/alexandre/Applications/sage/local/lib/python2.6/site-packages/sage/graphs/generic_graph.py:docstring of sage.graphs.generic_graph.GenericGraph.spanning_trees_count:27: (WARNING/2) Bullet list ends without a blank line; unexpected unindent.
/Users/alexandre/Applications/sage/devel/sage/doc/en/reference/sage/graphs/generic_graph.rst:7926: (WARNING/2) Duplicate explicit target name: "krg96".
/Users/alexandre/Applications/sage/devel/sage/doc/en/reference/sage/graphs/generic_graph.rst:7930: (WARNING/2) Duplicate explicit target name: "gyll93".
/Users/alexandre/Applications/sage/devel/sage/doc/en/reference/sage/graphs/generic_graph.rst:7926: WARNING: duplicate citation KRG96, other instance in /Users/alexandre/Applications/sage/devel/sage/doc/en/reference/sage/graphs/generic_graph.rst
/Users/alexandre/Applications/sage/devel/sage/doc/en/reference/sage/graphs/generic_graph.rst:7930: WARNING: duplicate citation GYLL93, other instance in /Users/alexandre/Applications/sage/devel/sage/doc/en/reference/sage/graphs/generic_graph.rst
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] structure                             urve_morphismtorcfield
writing additional files... genindex modindex search
copying images... media/homology/rp2.png media/homology/simplices.png media/homology/torus.png media/homology/klein.png media/homology/torus_labelled.png
copying static files... done
dumping search index... done
dumping object inventory... done
build succeeded, 8 warnings.
Build finished.  The built documents can be found in /Users/alexandre/Applications/sage/devel/sage/doc/output/html/en/reference

I'll try to fix them and upload a patch soon.

Changed 9 years ago by abmasse

Adds digraph.py and generic_graph.py in the doctree

comment:3 Changed 9 years ago by abmasse

  • Priority changed from major to minor
  • Status changed from needs_work to needs_review

Finally, since there weren't too many warnings, I think it's worth trying to fix all of them. This is done in the patch I just submitted.

To summarize, this patch adds the two files digraph.py and generic_graph.py to the reference manual and correct the less than 10 warnings that Sphinx was displaying.

Needs review!

comment:4 Changed 9 years ago by ncohen

Excellent ! Now we appear in the doc, and there are no warnings anymore.

Reviewing your patch, I met a few things that needed fixing in the docs, so if you agree with my modifications, you can set this to positive review.. Thank you !! :-)

Nathann

comment:5 Changed 9 years ago by abmasse

I agree with your changes ! I just hope it won't overlap with any other ticket.

By the way, did you test my patch ? When I created it, I used the sage-combinat queue and it worked fine, but here, when I tried to test your patch, I created a clone (with hg -clone t8513) to apply your patch on top of mine, but I got the following warning.

[~/Applications/sage/devel/sage-t8513]$ sage -docbuild reference html
sphinx-build -b html -d /Users/alexandre/Applications/sage/devel/sage/doc/output/doctrees/en/reference    /Users/alexandre/Applications/sage/devel/sage/doc/en/reference /Users/alexandre/Applications/sage/devel/sage/doc/output/html/en/reference
Running Sphinx v0.6.3
loading pickled environment... done
building [html]: targets for 2 source files that are out of date
updating environment: 0 added, 2 changed, 0 removed
reading sources... [100%] sage/graphs/graph                          
/Users/alexandre/Applications/sage/devel/sage/doc/en/reference/graphs.rst:4: (WARNING/2) toctree references unknown document u'sage/graphs/digraph'
/Users/alexandre/Applications/sage/devel/sage/doc/en/reference/graphs.rst:4: (WARNING/2) toctree references unknown document u'sage/graphs/generic_graph'
looking for now-outdated files... none found
pickling environment... done
checking consistency... done
preparing documents... done
writing output... [100%] sage/graphs/graph                           
writing additional files... genindex modindex search
copying static files... done
dumping search index... done
dumping object inventory... done
build succeeded, 2 warnings.

It's strange, it doesn't accept to add the two files to the doctree, as if the path was not correct when creating a clone, but was ok with sage-combinat. Does it work fine for you?

comment:6 Changed 9 years ago by ncohen

O_o

Odd.... I applied your patch as usual, and I mean by this not using sage-combinat. It applied fine, and passed all tests. After my modifications, same result O_o

Have you tried cloning the branch using sage -clone instead of doing it manually with hg ?

Nathann

comment:7 Changed 9 years ago by abmasse

No idea... Anyway, I'll try it with the sage-combinat queue to look at the documentation generated by your patch and it should be fine.

comment:8 Changed 9 years ago by mvngu

I have replaced ncohen's patch with one that has the ticket number in its commit message.

comment:9 follow-up: Changed 9 years ago by jhpalmieri

I don't know much about graph theory, but is there any sense to the current order in the reference manual? Wouldn't it make more sense for graph to come first, for example, rather than cliquer? I don't think alphabetical is the right approach: someone who wants to know about graphs in Sage may very well start at the first link in the "Graph Theory" chapter. (It might be even better to have an introductory section in the file devel/sage/doc/en/reference/graphs.rst (like matrices.rst, for example). That probably belongs on another ticket, though.)

comment:10 in reply to: ↑ 9 Changed 9 years ago by abmasse

Replying to jhpalmieri:

I don't know much about graph theory, but is there any sense to the current order in the reference manual? Wouldn't it make more sense for graph to come first, for example, rather than cliquer? I don't think alphabetical is the right approach: someone who wants to know about graphs in Sage may very well start at the first link in the "Graph Theory" chapter. (It might be even better to have an introductory section in the file devel/sage/doc/en/reference/graphs.rst (like matrices.rst, for example). That probably belongs on another ticket, though.)

I agree with you! There is some cleanup needed in the documentation. But maybe we should wait for it in another ticket. The goal of #8513 is to quickly make the graph documentation available so that one may check if Sphinx builds all right when reviewing related tickets. If it's ok for you, of course.

Sorry for being so late, by the way, Nathann, I'll try to finish this ticket today or at most, during the week.

comment:11 Changed 9 years ago by mvngu

  • Authors set to Alexandre Blondin Massé, Nathann Cohen
  • Reviewers set to Minh Van Nguyen

Replying to jhpalmieri:

I don't know much about graph theory, but is there any sense to the current order in the reference manual?

No, I don't think so. The current organization is a mess.

Wouldn't it make more sense for graph to come first, for example, rather than cliquer?

Yes. The current organization of the graph theory modules, as they appear in the reference manual, is rather unusual. For example, the interface to cliquer appears as the very first link. One would expect something along the following line to be more natural:

  • undirected graph
  • digraph
  • generic graph
  • applications of graph theory
  • fast compiled graph
  • etc.

I have uploaded a reviewer patch that does something about this. With my reviewer patch, the organization of the graph theory modules should be more systematic.

I don't think alphabetical is the right approach: someone who wants to know about graphs in Sage may very well start at the first link in the "Graph Theory" chapter. (It might be even better to have an introductory section in the file devel/sage/doc/en/reference/graphs.rst (like matrices.rst, for example). That probably belongs on another ticket, though.)

Nod. This needs to wait for another ticket.

After applying trac_8513_graph_theory_documentation-abm.patch and trac_8513_graph_theory_documentation-smallfixes.patch, I then rebuilt the whole Sage documentation. A long doctest of the whole Sage library resulted in the following failure:

sage -t -long devel/sage-main/sage/misc/sagedoc.py
**********************************************************************
File "/dev/shm/mvngu/sandbox/sage-4.3.5-8513-graph-doc/devel/sage-main/sage/misc/sagedoc.py", line 892:
    sage: len(search_doc('tree', whole_word=True, interact=False).splitlines()) < 100
Expected:
    True
Got:
    False

This is due to adding two new files to the reference manual that happen to be about graph theory, hence the above search returns more matches for the word "tree". My reviewer patch should fix this doctest failure. It needs reviewing by anyone but me. I'm happy with both abmasse and ncohen's patches. Apply patches in the following order:

  1. trac_8513_graph_theory_documentation-abm.patch
  2. trac_8513_graph_theory_documentation-smallfixes.patch
  3. trac_8513-reviewer.patch

comment:12 Changed 9 years ago by abmasse

I'm running the longtest on all sage right now. Giving some feedback later tonight.

comment:13 follow-up: Changed 9 years ago by jhpalmieri

  • Reviewers changed from Minh Van Nguyen to Minh Van Nguyen, John Palmieri
  • Status changed from needs_review to needs_work

My reviewer patch should fix this doctest failure. It needs reviewing by anyone but me.

Your patch fixes the problem, but it doesn't really fit in sagedoc.py: the doctests there are supposed to contrast what happens if whole_word=True is not used (more than 2000 matches) vs. when it is (used to be fewer than 100 matches). How about changing it to len(...) < 150 or len(...) < 200?

Changed 9 years ago by mvngu

reviewer patch

comment:14 in reply to: ↑ 13 Changed 9 years ago by mvngu

  • Status changed from needs_work to needs_review

Replying to jhpalmieri:

Your patch fixes the problem, but it doesn't really fit in sagedoc.py: the doctests there are supposed to contrast what happens if whole_word=True is not used (more than 2000 matches) vs. when it is (used to be fewer than 100 matches). How about changing it to len(...) < 150 or len(...) < 200?

I see what you mean. Performing a search with whole_word=False results in too many matches. When using whole_word=True, we want to provide an upper bound on the number of matches. I have modified the reviewer patch accordingly.

comment:15 follow-ups: Changed 9 years ago by jhpalmieri

I'm happy with the reviewer patch now. After rebuilding the docs, all tests pass on sage.math. Does that mean the whole ticket gets a positive review?

comment:16 in reply to: ↑ 15 Changed 9 years ago by abmasse

Replying to jhpalmieri:

I'm happy with the reviewer patch now. After rebuilding the docs, all tests pass on sage.math. Does that mean the whole ticket gets a positive review?

Sure, if you're ok with the result. It's even better if the final reviewer isn't Nathann nor me.

comment:17 in reply to: ↑ 15 Changed 9 years ago by mvngu

Replying to jhpalmieri:

I'm happy with the reviewer patch now. After rebuilding the docs, all tests pass on sage.math. Does that mean the whole ticket gets a positive review?

I think so. Cleaning up the modules added by this ticket to the reference manual can be split off to an enhancement ticket.

comment:18 Changed 9 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

See #8684 for a follow-up ticket, the purpose of which is to organize the graph theory stuff in the reference manual. I'm not going to do this myself, since I don't know enough about graph theory in general, or the graph theory in Sage, to know how to organize it.

comment:19 Changed 9 years ago by jhpalmieri

  • Merged in set to sage-4.4.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

Merged in 4.4.alpha0:

  • trac_8513_graph_theory_documentation-abm.patch
  • trac_8513_graph_theory_documentation-smallfixes.patch
  • trac_8513-reviewer.patch
Note: See TracTickets for help on using tickets.