Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-generated thematic index of functions #19061

Closed
nathanncohen mannequin opened this issue Aug 20, 2015 · 42 comments
Closed

Auto-generated thematic index of functions #19061

nathanncohen mannequin opened this issue Aug 20, 2015 · 42 comments

Comments

@nathanncohen
Copy link
Mannequin

nathanncohen mannequin commented Aug 20, 2015

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

Depends on #19067

CC: @dcoudert @jm58660 @sagetrac-dlucas @johanrosenkilde @dimpase @mezzarobba

Component: documentation

Author: Nathann Cohen

Branch/Commit: 14b672d

Reviewer: David Coudert

Issue created by migration from https://trac.sagemath.org/ticket/19061

@nathanncohen nathanncohen mannequin added this to the sage-6.9 milestone Aug 20, 2015
@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 20, 2015

Branch: public/19061

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 20, 2015

New commits:

bf85f4dtrac #19061: Auto-generated thematic index of functions

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 20, 2015

Commit: bf85f4d

@nathanncohen nathanncohen mannequin added the s: needs review label Aug 20, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2015

Changed commit from bf85f4d to c6db853

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 20, 2015

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

c6db853trac #19061: Auto-generated thematic index of functions

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Aug 21, 2015

comment:4

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

@jm58660 jm58660 mannequin added s: needs work and removed s: needs review labels Aug 21, 2015
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2015

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

53b8469trac #19061: Merged with 6.9.beta3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2015

Changed commit from c6db853 to 53b8469

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 21, 2015

comment:6

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

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Aug 21, 2015

comment:7

Replying to @nathanncohen:

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 "chromatic_number() Returns the minimal number of colors needed to color the vertices", i.e. forgetting the ending "of the graph G.". And next one is "chromatic_polynomial() 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.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 21, 2015

comment:8

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 "chromatic_number() Returns the minimal number of colors needed to color the vertices", i.e. forgetting the ending "of the graph G.". And next one is "chromatic_polynomial() 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

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Aug 21, 2015

comment:9

Replying to @nathanncohen:

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

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 21, 2015

comment:10

I will probably write a fix for that today. I will add a comment here when I do.

Done at #19067.

Nathann

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Aug 22, 2015

Dependencies: #19067

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 31, 2015

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

56430d1trac #19061: Merged with 6.9.beta4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 31, 2015

Changed commit from 53b8469 to 56430d1

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 9, 2015

comment:13

(the dependency of this ticket is now reviewed)

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2015

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

d1bda42trac #19061: Merged with 6.9.rc0

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2015

Changed commit from 56430d1 to d1bda42

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 30, 2015

comment:15

Rebased against the latest beta.

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

@dcoudert
Copy link
Contributor

comment:16

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.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 30, 2015

comment:17
  • 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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2015

Changed commit from d1bda42 to b52a8bc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2015

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

b52a8bctrac #19061: utf8 for graph.py

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 30, 2015

comment:19

Question asked at https://groups.google.com/d/topic/sage-devel/lNNEQvBlYKc/discussion

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2015

Changed commit from b52a8bc to 8fa7a28

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 30, 2015

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

8fa7a28trac #19061: Workaround Cython injection

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 30, 2015

comment:21

Fixed (thanks to Jeroen)

@dcoudert
Copy link
Contributor

comment:22

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.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Sep 30, 2015

comment:23

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

@dcoudert
Copy link
Contributor

dcoudert commented Oct 9, 2015

comment:24

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?

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 9, 2015

comment:25

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 9, 2015

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 9, 2015

Changed commit from 8fa7a28 to 14b672d

@dcoudert
Copy link
Contributor

comment:27

For me the patch is good to go, but the patchbot is complaining about something I don't understand. Can you check that?

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 10, 2015

comment:28

I have no clue either O_o

@johanrosenkilde
Copy link
Contributor

comment:29

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

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 10, 2015

comment:30

I see. Thanks.

Nathann

@dcoudert
Copy link
Contributor

Reviewer: David Coudert

@dcoudert
Copy link
Contributor

comment:31

So then let's go.
David.

@nathanncohen
Copy link
Mannequin Author

nathanncohen mannequin commented Oct 10, 2015

comment:32

Thaaaaaaaaaaaaanks !!!!

@vbraun
Copy link
Member

vbraun commented Oct 12, 2015

Changed branch from public/19061 to 14b672d

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants