Opened 4 years ago

Closed 4 years ago

#19061 closed enhancement (fixed)

Auto-generated thematic index of functions

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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 try-and-fail, and corner cases, and bad surprises. It is totally open to disccussion, but know that there may be a reason (i.e. a use-case) 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 4 years ago by ncohen

  • Branch set to public/19061
  • Cc dimpase added
  • Commit set to bf85f4da13a5cfcf1563ca2a8862d769578ff936
  • Status changed from new to needs_review

New commits:

bf85f4dtrac #19061: Auto-generated thematic index of functions

comment:2 Changed 4 years ago by git

  • Commit changed from bf85f4da13a5cfcf1563ca2a8862d769578ff936 to c6db8534c85b16c336ea78085975f8b8edc98cec

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

c6db853trac #19061: Auto-generated thematic index of functions

comment:3 Changed 4 years ago by mmezzarobba

  • Cc mmezzarobba added

comment:4 follow-up: Changed 4 years ago by jmantysalo

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

  • Commit changed from c6db8534c85b16c336ea78085975f8b8edc98cec to 53b8469b104942a4030d68a08df662471bdb8f6f

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

53b8469trac #19061: Merged with 6.9.beta3

comment:6 in reply to: ↑ 4 ; follow-up: Changed 4 years ago by ncohen

  • 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 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".

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 ; follow-up: Changed 4 years ago by jmantysalo

Replying to ncohen:

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".

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 ; follow-ups: Changed 4 years ago by ncohen

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

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 :=) )

Last edited 4 years ago by jmantysalo (previous) (diff)

comment:10 in reply to: ↑ 8 Changed 4 years ago by ncohen

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

  • Dependencies set to #19067

comment:12 Changed 4 years ago by git

  • Commit changed from 53b8469b104942a4030d68a08df662471bdb8f6f to 56430d13d608ab79ad36f6866988afe473b9e0df

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

56430d1trac #19061: Merged with 6.9.beta4

comment:13 Changed 4 years ago by ncohen

(the dependency of this ticket is now reviewed)

comment:14 Changed 4 years ago by git

  • Commit changed from 56430d13d608ab79ad36f6866988afe473b9e0df to d1bda42bf0d94c52b24378a6bc5283ce22ec8726

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

d1bda42trac #19061: Merged with 6.9.rc0

comment:15 Changed 4 years ago by ncohen

Rebased against the latest beta.

If anybody has some time to review this, it would be cool.

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

I had a look at the resulting html page and we have some problems:

  • Return the value of Lovász theta-function 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 ncohen

  • Return the value of Lovász theta-function 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 git

  • Commit changed from d1bda42bf0d94c52b24378a6bc5283ce22ec8726 to b52a8bce2a3cabc089ae6a4b91e38a38af94e9d8

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

b52a8bctrac #19061: utf8 for graph.py

comment:20 Changed 4 years ago by git

  • Commit changed from b52a8bce2a3cabc089ae6a4b91e38a38af94e9d8 to 8fa7a280222ee3bccdc91ecce9f58ed88cd1c6f1

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

8fa7a28trac #19061: Workaround Cython injection

comment:21 Changed 4 years ago by ncohen

Fixed (thanks to Jeroen)

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

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 ncohen

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 "NP-Hard problems" here if you prefer.

Nathann

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

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 ncohen

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 git

  • Commit changed from 8fa7a280222ee3bccdc91ecce9f58ed88cd1c6f1 to 14b672decf9e60cf6a00593e15d4dcd0af5c4df2

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

72bd598trac #19061: Merged with 6.9.rc3
14b672dtrac #19061: Split a line of doc

comment:27 Changed 4 years ago by dcoudert

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 ncohen

I have no clue either O_o

comment:29 Changed 4 years ago by jsrn

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 ncohen

I see. Thanks.

Nathann

comment:31 Changed 4 years ago by dcoudert

  • 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 ncohen

Thaaaaaaaaaaaaanks !!!!

comment:33 Changed 4 years ago by vbraun

  • Branch changed from public/19061 to 14b672decf9e60cf6a00593e15d4dcd0af5c4df2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.