Opened 5 years ago
Closed 4 years ago
#19061 closed enhancement (fixed)
Autogenerated thematic index of functions
Reported by:  ncohen  Owned by:  

Priority:  major  Milestone:  sage6.9 
Component:  documentation  Keywords:  
Cc:  dcoudert, jmantysalo, dlucas, jsrn, dimpase, mmezzarobba  Merged in:  
Authors:  Nathann Cohen  Reviewers:  David Coudert 
Report Upstream:  N/A  Work issues:  
Branch:  14b672d (Commits)  Commit:  14b672decf9e60cf6a00593e15d4dcd0af5c4df2 
Dependencies:  #19067  Stopgaps: 
Description
This branch extends the feature added by #18926, so that creating thematic index of functions is more reliable. In particular, we are now sure that *all* methods of a class appear in the index (no risk to 'forget' one), and we can pick a category for each of them.
The interface is the result of a *LOT* of tryandfail, and corner cases, and bad surprises. It is totally open to disccussion, but know that there may be a reason (i.e. a usecase) explaining which this part is written that way. I am not very proud of everything it does either.
But it will definitely help us.
Nathann
Change History (33)
comment:1 Changed 5 years ago by
 Branch set to public/19061
 Cc dimpase added
 Commit set to bf85f4da13a5cfcf1563ca2a8862d769578ff936
 Status changed from new to needs_review
comment:2 Changed 5 years ago by
 Commit changed from bf85f4da13a5cfcf1563ca2a8862d769578ff936 to c6db8534c85b16c336ea78085975f8b8edc98cec
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c6db853  trac #19061: Autogenerated thematic index of functions

comment:3 Changed 5 years ago by
 Cc mmezzarobba added
comment:4 followup: ↓ 6 Changed 5 years ago by
 Status changed from needs_review to needs_work
Needs rebasing > needs_work.
I won't review this because I don't know about Sphinx internals.
This patch clearly makes Sage better than it currently is, so I think this should be added. So this is just a suggestion: If we still have for example is_chain_of_poset()
in the index, and not is_chain_of_poset(elms)
, should it be possible to add different onliner to index than what is at the beginning of function? Like "Return True if given iterable is a chain of the poset." instead of "Return True if elms
is a chain of the poset".
comment:5 Changed 5 years ago by
 Commit changed from c6db8534c85b16c336ea78085975f8b8edc98cec to 53b8469b104942a4030d68a08df662471bdb8f6f
Branch pushed to git repo; I updated commit sha1. New commits:
53b8469  trac #19061: Merged with 6.9.beta3

comment:6 in reply to: ↑ 4 ; followup: ↓ 7 Changed 5 years ago by
 Status changed from needs_work to needs_review
So this is just a suggestion: If we still have for example
is_chain_of_poset()
in the index, and notis_chain_of_poset(elms)
, should it be possible to add different onliner to index than what is at the beginning of function? Like "Return True if given iterable is a chain of the poset." instead of "Return True ifelms
is a chain of the poset".
I can imagine that such a feature could be useful eventually. One way to make it available would be through the @doc_index
constructor, which would then take both a index name and a description as an argument.
Nathann
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 5 years ago by
Replying to ncohen:
So this is just a suggestion: If we still have for example
is_chain_of_poset()
in the index, and notis_chain_of_poset(elms)
, should it be possible to add different onliner to index than what is at the beginning of function? Like "Return True if given iterable is a chain of the poset." instead of "Return True ifelms
is a chain of the poset".I can imagine that such a feature could be useful eventually. One way to make it available would be through the
@doc_index
constructor, which would then take both a index name and a description as an argument.
Yes, that sounds good. Without this second argument it could work just like now.
I compiled this and got strange results. graph.html
starts with "Algorithmically hard stuff", i.e. uses alphabetical list. Should be possible to change it.
Then, it starts right with "<link>chromatic_number()</link> Returns the minimal number of colors needed to color the vertices", i.e. forgetting the ending "of the graph G.". And next one is "<link>chromatic_polynomial()</link> chromatic_polynomial(G, return_tree_basis=False)".
Do others get similar results? Can be just some Spinx error and maybe I must recompile or something.
comment:8 in reply to: ↑ 7 ; followups: ↓ 9 ↓ 10 Changed 5 years ago by
Hello,
I compiled this and got strange results.
graph.html
starts with "Algorithmically hard stuff", i.e. uses alphabetical list. Should be possible to change it.
Yeah, I thought about a way to enforce an ordering. It is the kind of things that I expect we will have some day, but I did not think it so urgent that it needed to be implemented here. A lexicographic ordering is not so bad, really.
Then, it starts right with "<link>chromatic_number()</link> Returns the minimal number of colors needed to color the vertices", i.e. forgetting the ending "of the graph G.". And next one is "<link>chromatic_polynomial()</link> chromatic_polynomial(G, return_tree_basis=False)".
Do others get similar results? Can be just some Spinx error and maybe I must recompile or something.
I did not notice, but I expect that you are right. I know how I wrote the code that extracts this text and it only takes 'the first line', not the first sentence. It is not related to this ticket thought, it's a code that is already in Sage.
I will probably write a fix for that today. I will add a comment here when I do.
Nathann
comment:9 in reply to: ↑ 8 Changed 5 years ago by
Replying to ncohen:
I know how I wrote the code that extracts this text and it only takes 'the first line', not the first sentence. It is not related to this ticket thought, it's a code that is already in Sage.
OK. That's what I guessed that happened. Also you should check that both """
and r"""
works.
I will probably write a fix for that today. I will add a comment here when I do.
OK. I will read it later, as I will be hearing a dissertation a hour from now on. (http://tampub.uta.fi/handle/10024/97895  last two papers use Sage :=)
)
comment:10 in reply to: ↑ 8 Changed 5 years ago by
I will probably write a fix for that today. I will add a comment here when I do.
Done at #19067.
Nathann
comment:11 Changed 5 years ago by
 Dependencies set to #19067
comment:12 Changed 4 years ago by
 Commit changed from 53b8469b104942a4030d68a08df662471bdb8f6f to 56430d13d608ab79ad36f6866988afe473b9e0df
Branch pushed to git repo; I updated commit sha1. New commits:
56430d1  trac #19061: Merged with 6.9.beta4

comment:13 Changed 4 years ago by
(the dependency of this ticket is now reviewed)
comment:14 Changed 4 years ago by
 Commit changed from 56430d13d608ab79ad36f6866988afe473b9e0df to d1bda42bf0d94c52b24378a6bc5283ce22ec8726
Branch pushed to git repo; I updated commit sha1. New commits:
d1bda42  trac #19061: Merged with 6.9.rc0

comment:15 Changed 4 years ago by
Rebased against the latest beta.
If anybody has some time to review this, it would be cool.
comment:16 followup: ↓ 17 Changed 4 years ago by
I had a look at the resulting html page and we have some problems:
Return the value of LovÃ¡sz thetafunction of graph
 This is weird:
chromatic_polynomial(G, return_tree_basis=False) File: sage/graphs/chrompoly.pyx (starting at line 28)
is_long_antihole_free(g, certificate=False) File: sage/graphs/weakly_chordal.pyx (starting at line 216)
is_cartesian_product(g, certificate=False, relabeling=False) File: sage/graphs/graph_decompositions/graph_products.pyx (starting at line 137)
 etc.
comment:17 in reply to: ↑ 16 Changed 4 years ago by
Return the value of LovÃ¡sz thetafunction of graph
Just an utf8 problem. Fixed.
 This is weird:
chromatic_polynomial(G, return_tree_basis=False) File: sage/graphs/chrompoly.pyx (starting at line 28)
What the hell O_o
sage: print Graph.chromatic_polynomial.__doc__ chromatic_polynomial(G, return_tree_basis=False) File: sage/graphs/chrompoly.pyx (starting at line 28) Computes the chromatic polynomial of the graph G. ... sage: print Graph.vertices.__doc__ Return a list of the vertices. ...
Nathann
comment:18 Changed 4 years ago by
 Commit changed from d1bda42bf0d94c52b24378a6bc5283ce22ec8726 to b52a8bce2a3cabc089ae6a4b91e38a38af94e9d8
Branch pushed to git repo; I updated commit sha1. New commits:
b52a8bc  trac #19061: utf8 for graph.py

comment:19 Changed 4 years ago by
Question asked at https://groups.google.com/d/topic/sagedevel/lNNEQvBlYKc/discussion
comment:20 Changed 4 years ago by
 Commit changed from b52a8bce2a3cabc089ae6a4b91e38a38af94e9d8 to 8fa7a280222ee3bccdc91ecce9f58ed88cd1c6f1
Branch pushed to git repo; I updated commit sha1. New commits:
8fa7a28  trac #19061: Workaround Cython injection

comment:21 Changed 4 years ago by
Fixed (thanks to Jeroen)
comment:22 followup: ↓ 23 Changed 4 years ago by
Much better now. I assume that we can let the ordering problem (starting with algorithmic hard stuff is not necessarily the best choice) for another patch.
comment:23 in reply to: ↑ 22 Changed 4 years ago by
I assume that we can let the ordering problem (starting with algorithmic hard stuff is not necessarily the best choice) for another patch.
Yep, I think so too. Or I can rename it to "NPHard problems" here if you prefer.
Nathann
comment:24 followup: ↓ 25 Changed 4 years ago by
in the generated list for graph.py, I see:
strong_orientation() Returns a strongly connected orientation of the current graph. See also the Wikipedia article Strongly_connected_component.
with a link to the corresponding wikipedia page. For me this is acceptable but is this behavior expected?
comment:25 in reply to: ↑ 24 Changed 4 years ago by
Hello,
in the generated list for graph.py, I see:
strong_orientation() Returns a strongly connected orientation of the current graph. See also the Wikipedia article Strongly_connected_component.with a link to the corresponding wikipedia page. For me this is acceptable but is this behavior expected?
It is only because the docstring of strong_orientation
contains this additional line in its first paragraph. Can be solved by splitting it into two lines.
Nathann
comment:26 Changed 4 years ago by
 Commit changed from 8fa7a280222ee3bccdc91ecce9f58ed88cd1c6f1 to 14b672decf9e60cf6a00593e15d4dcd0af5c4df2
comment:27 Changed 4 years ago by
For me the patch is good to go, but the patchbot is complaining about something I don't understand. Can you check that?
comment:28 Changed 4 years ago by
I have no clue either O_o
comment:29 Changed 4 years ago by
It seems to be complaining that sage.misc.collections
is, after the patch, being imported per default on Sage startup, and this wasn't the case before. The import per default is done since sage.misc.rest_index_of_methods
is being imported per default, and it includes a line to import the sage.misc.collections
.
Sage takes a long time to start, and to try to not aggravate that in the future, the patchbot checks whether a patch will make new modules load at startup.
I presume that sage.misc.rest_index_of_methods
more or less has to load at startup. And I assume that the sage.misc.collections.defaultdict
is being used to good effect. In this case, the patchbot warning can just be ignored.
Best, Johan
comment:30 Changed 4 years ago by
I see. Thanks.
Nathann
comment:31 Changed 4 years ago by
 Reviewers set to David Coudert
 Status changed from needs_review to positive_review
So then let's go. David.
comment:32 Changed 4 years ago by
Thaaaaaaaaaaaaanks !!!!
comment:33 Changed 4 years ago by
 Branch changed from public/19061 to 14b672decf9e60cf6a00593e15d4dcd0af5c4df2
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #19061: Autogenerated thematic index of functions