Auto-generated index of functions

[nathanncohen]

This branch implements a new function named gen_rest_table_index which generates an index of the functions given as arguments.

It can also generate the list of all methods of a given class A.

With this functions, it is then very easy to add an index of functions in a Sage module, that does not have to be kept updated manually.

Nathann

CC: @dcoudert @jm58660 @sagetrac-dlucas

Component: documentation

Author: Nathann Cohen

Branch/Commit: 37011a4

Reviewer: Johan Sebastian Rosenkilde Nielsen

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

[nathanncohen]

New commits:

b231896

trac #18926: Auto-generated index of functions

[nathanncohen]

Commit: b231896

[nathanncohen]

Branch: public/18926

[jm58660]

comment:2

CC'ing myself because of #18636.

[sagetrac-git]

Changed commit from b231896 to e1a3241

[sagetrac-git]

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

e1a3241

trac #18926: Table from #18636

[jm58660]

comment:5

Just to make sure: there is no way to skip all deprecated functions? On posets.py compare to_graph() and meet_matrix().

The code seems to be clear. However, I know nothing about introspection in python, so hopefully somebody else will review this.

[nathanncohen]

comment:6

Just to make sure: there is no way to skip all deprecated functions?

I already filter out those that I can identify. If you see a way to remove others, we can add it.

Nathann

[johanrosenkilde]

comment:7

Hi Nathann

Clever idea and nice code. I like it.

A few small comments:

• The doc string doesn't specify that you can give the function a module (only a class).

• You could add a doc-test that passing a class or module also works. For good
  quine humour, it could be:

  sage: gen_rest_table_index(sage.misc.rest_index_of_methods)
  ....

• What exactly do you filter out with the inspect.getmodule(root) == inspect.getmodule(f) check?

  This function would be nice to use in catalogues, such as
  sage.coding.codes_catalog. But currently one can't due to the above line.
  Could that, however, possibly give problems with references given in the
  one-line description of a function from another module?

• The function doesn't seem to do very interesting things for classes which have
  inherited most of their methods. Is this by design? It is unfortunate wrt. the
  category framework which garbles parent classes. For instance, running the
  function on FinitePosets returns nothing.

  Instead of using root.__dict__, then dir(root) would find everything and
  his grandmother. Could that be more clever? Here the check
  inspect.getmodule(root) == ... also seems to get in the way for listing
  class methods.

Johan

[nathanncohen]

comment:9

Hellooooooooooo !

Clever idea and nice code. I like it.

Thanks. I like it too :-P

• The doc string doesn't specify that you can give the function a module (only a class).

Sorry. Fixed.

• You could add a doc-test that passing a class or module also works. For good
  quine humour, it could be:

  sage: gen_rest_table_index(sage.misc.rest_index_of_methods)
  ....

Good idea, done.

• What exactly do you filter out with the inspect.getmodule(root) == inspect.getmodule(f) check?

The imported modules. If you remove it, you will see gen_rest_table_index appear in the documentation of sage.combinat.designs.difference_families.

• The function doesn't seem to do very interesting things for classes which have
  inherited most of their methods. Is this by design?

It was designed to only show the 'first-level' functions, i.e. not the inherited ones.

Instead of using root.__dict__, then dir(root) would find everything and
his grandmother. Could that be more clever? Here the check
inspect.getmodule(root) == ... also seems to get in the way for listing
class methods.

If you find a nice way to make it work, I have no objection for as long as both remain available. The list of methods of 'graph' is already long enough:

http://doc.sagemath.org/html/en/reference/graphs/sage/graphs/graph.html

You don't want to see its functions mixed with those:

http://doc.sagemath.org/html/en/reference/graphs/sage/graphs/generic_graph.html

Of course this index of functions is 'hand-made' so the example does not apply. But there are other places where I would like to have an index of functions: many classes inherit from IncidenceStructure, and I would not want to see the methods of IncidenceStructure repeated everywhere.

In this situation, it would be more efficient to list only the first-level methods, and have links saying what this class inherits from, so that users can find the other methods easily.

This way we also avoid seeing the usual broken functions "which should not be inherited but are nevertheless" like

sage: Partitions(4).base_ring
sage: Partitions(4).objgen
sage: Partitions(4).objgens
...

Nathann

[sagetrac-git]

Changed commit from e1a3241 to eb276db

[sagetrac-git]

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

5a96f89	trac #18926: Merged with 6.8.rc1
eb276db	trac #18926: Reviewer's remarks

[johanrosenkilde]

comment:12

Helloooooooooooooooo Nathannnnnnnn,

For some reason, I can't pull from trac.sagemath.org right now - connection timeout. So I can't get your newest version and run the tests.

I see what you're saying about the locally defined functions. To support
catalogues like sage.coding.codes_catalog, a flag only_local=True could be
added: usually, we wan't to filter out imported or inherited functions, but
sometimes we might want them in. I don't think it complicates the function
detrimentally.

I'd push this change myself if trac was working for me right now.

I also wanted to suggest some further polishing of the docstrings. I'll push that as soon as I can again connect to trac.sagemath.org.

Johan

[sagetrac-git]

Changed commit from eb276db to 5fc4e9e

[sagetrac-git]

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

69f1667	Don't crash if there's no docstring
a812984	Some doc-string clarifications
614a04d	Add the only_local keyword to allow use in catalog modules
5fc4e9e	Add doctest for self-inclusion when only_local=False

[johanrosenkilde]

comment:14

Hi Nathann,

Now the trac firewall has been doused, I've corrected the small things I mentioned. From my side, I give the ticket a green light now. So you can review my small changes :-)

Johan

[nathanncohen]

comment:15

Helloooooo Johan,

I have several remarks about your commits:

• If you build the documentation of reference/misc, you will notice that the description (in the index) of gen_rest_table_index is unfinished: only the first line is taken into account (see http://doc.sagemath.org/html/en/developer/coding_basics.html#the-docstring-of-a-function-content)

• Wrong alignment of a paragraph in the documentation of only_local, and some latex code which should be surrounded by two backticks instead of only one.

• About only_local: I do not understand this doctest about not including gen_rest_table_index in the index of ... its own module. Why wouldn't only_local=False print always more than only_local=True in general? This does not make sense to me.

• I do not understand your mention of 'static' methods.

• Above the 'input' block, a lone line applies to 'classes' only, and does not take the value 'only_local' into account.

Nathann

[sagetrac-git]

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

9325bb3

Reviewer^2 fixes

[sagetrac-git]

Changed commit from 5fc4e9e to 9325bb3

[sagetrac-git]

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

3a9e3b5

Yet another docstring fix

[sagetrac-git]

Changed commit from 9325bb3 to 3a9e3b5

[johanrosenkilde]

comment:18

Hiiiiii Nathann,

• If you build the documentation of reference/misc...

Oh, sorry, didn't check that. Fixed.

• Wrong alignment of a paragraph in the documentation of only_local, and some latex code which should be surrounded by two backticks instead of only one.

Fixed.

• About only_local: I do not understand this doctest about not including gen_rest_table_index in the index of ... its own module. Why wouldn't only_local=False print always more than only_local=True in general? This does not make sense to me.

Yes, it's slightly bizarre. Say you have a catalog module that you wish to have
an index for. Then you import gen_rest_table_index and you call it with
only_local=False: you want all the non-local functions, except
gen_rest_table_index itself. The funny test is then simply to see that this
is filtered out, without having to rely on another module.

• I do not understand your mention of 'static' methods.

The function only extracts methods of a class which have been decorated with
@staticmethod. It will not extract methods on instances of the class, i.e.
the normal methods. This was already the semantics in your original version.

• Above the 'input' block, a lone line applies to 'classes' only, and does not take the value 'only_local' into account.

This line was just to clarify the semantics of the function wrt. classes, since
this surprised me when I first reviewed the ticket. I never intended
only_local to have any effect when the function is called on classes.

Johan

---

New commits:

3a9e3b5

Yet another docstring fix

---

New commits:

3a9e3b5

Yet another docstring fix

[nathanncohen]

comment:19

Helloooooooo,

Yes, it's slightly bizarre. Say you have a catalog module that you wish to have
an index for. Then you import gen_rest_table_index and you call it with
only_local=False: you want all the non-local functions, except
gen_rest_table_index itself. The funny test is then simply to see that this
is filtered out, without having to rely on another module.

Oh okay, it is true that somehow this function has a 'special behaviour'. Would you see anything wrong in filtering it out with f is not gen_rest_table_index instead of checking its module?

The function only extracts methods of a class which have been decorated with
@staticmethod.

Whaaaaat? O_o

None of the methods of IncidenceStructure are decorated like that. And they appear in the list O_o

This line was just to clarify the semantics of the function wrt. classes, since
this surprised me when I first reviewed the ticket. I never intended
only_local to have any effect when the function is called on classes.

Oh. What about renaming it to 'only_local_functions` to make it plain(er) that it does not apply to methods?

Nathann

[johanrosenkilde]

comment:20

Oh okay, it is true that somehow this function has a 'special behaviour'. Would you see anything wrong in filtering it out with f is not gen_rest_table_index instead of checking its module?

No, that's probably simpler. I did the other check in case more functions are added to the module, but now I realise that even if that happens, multiple such functions are probably not imported into the same module.

The function only extracts methods of a class which have been decorated with
@staticmethod.

Whaaaaat? O_o

None of the methods of IncidenceStructure are decorated like that. And they appear in the list O_o

Ok... It seems I've been on drugs (benadryl). I know that it was some class in combinat.posets that made me think this, and I though I tested it. I must have messed it up with the inherited-methods-not-included thing...

This line was just to clarify the semantics of the function wrt. classes, since
this surprised me when I first reviewed the ticket. I never intended
only_local to have any effect when the function is called on classes.

Oh. What about renaming it to 'only_local_functions` to make it plain(er) that it does not apply to methods?

Yeah, that's a good idea.

[sagetrac-git]

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

62fae3d

More reviewer^2 comments

[sagetrac-git]

Changed commit from 3a9e3b5 to 62fae3d

[sagetrac-git]

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

37011a4

trac #18926: Cleaning

[sagetrac-git]

Changed commit from 62fae3d to 37011a4

[nathanncohen]

comment:23

Hellooooo !

I cleaned/simplified a couple of things. Also, it seems that this code catalog of yours did not compile anymore and the functions appeared twice. Is it okay for you now?

I do not see anything to add, on my side.

Nathann

[johanrosenkilde]

Reviewer: Johan Sebastian Rosenkilde Nielsen

[johanrosenkilde]

comment:24

Sorry for the mess. Yes, it is ok for me now.

[nathanncohen]

comment:25

Exxxxxxxxxcellent. Thanks. We now have free index of functions, nothing to do by hand. Cool.

Nathann

[nathanncohen]

comment:26

And thank you very much for your help!!!

Nathann

[johanrosenkilde]

comment:27

Yes it's very nice :-) I though several times that such a thing would be good to have but I was a bit daunted with the prospects of picking into Sphinx internals. Your solution is very neat.

[vbraun]

Changed branch from public/18926 to 37011a4