Opened 3 years ago

Closed 3 years ago

#18926 closed enhancement (fixed)

Auto-generated index of functions

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.8
Component: documentation Keywords:
Cc: dcoudert, jmantysalo, dlucas Merged in:
Authors: Nathann Cohen Reviewers: Johan Sebastian Rosenkilde Nielsen
Report Upstream: N/A Work issues:
Branch: 37011a4 (Commits) Commit: 37011a4187cbd3d91ca68224a82151666682dd67
Dependencies: Stopgaps:

Description

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

Change History (28)

comment:1 Changed 3 years ago by ncohen

  • Branch set to public/18926
  • Commit set to b23189624b8e9c913af0140fb0a036291340ce8b
  • Status changed from new to needs_review

New commits:

b231896trac #18926: Auto-generated index of functions

comment:2 Changed 3 years ago by jmantysalo

  • Cc jmantysalo added

CC'ing myself because of #18636.

comment:3 Changed 3 years ago by dlucas

  • Cc dlucas added

comment:4 Changed 3 years ago by git

  • Commit changed from b23189624b8e9c913af0140fb0a036291340ce8b to e1a3241375269c7f8bff7431e5bceee79f094641

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

e1a3241trac #18926: Table from #18636

comment:5 follow-up: Changed 3 years ago by jmantysalo

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.

comment:6 in reply to: ↑ 5 Changed 3 years ago by ncohen

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

comment:7 follow-up: Changed 3 years ago by jsrn

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

comment:8 Changed 3 years ago by jsrn

  • Status changed from needs_review to needs_info

comment:9 in reply to: ↑ 7 Changed 3 years ago by ncohen

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

comment:10 Changed 3 years ago by git

  • Commit changed from e1a3241375269c7f8bff7431e5bceee79f094641 to eb276db9b51097b202cc9a5e1f71f1f9ca599925

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

5a96f89trac #18926: Merged with 6.8.rc1
eb276dbtrac #18926: Reviewer's remarks

comment:11 Changed 3 years ago by ncohen

  • Status changed from needs_info to needs_review

comment:12 Changed 3 years ago by jsrn

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

comment:13 Changed 3 years ago by git

  • Commit changed from eb276db9b51097b202cc9a5e1f71f1f9ca599925 to 5fc4e9e9bffe705a66386bce27655da00727bd1a

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

69f1667Don't crash if there's no docstring
a812984Some doc-string clarifications
614a04dAdd the only_local keyword to allow use in catalog modules
5fc4e9eAdd doctest for self-inclusion when only_local=False

comment:14 Changed 3 years ago by jsrn

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

comment:15 follow-up: Changed 3 years ago by ncohen

Helloooooo Johan,

I have several remarks about your commits:

  • 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

comment:16 Changed 3 years ago by git

  • Commit changed from 5fc4e9e9bffe705a66386bce27655da00727bd1a to 9325bb36ef1e6d0a16a986c7a6ecf20c104fef40

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

9325bb3Reviewer^2 fixes

comment:17 Changed 3 years ago by git

  • Commit changed from 9325bb36ef1e6d0a16a986c7a6ecf20c104fef40 to 3a9e3b5cbb00db4ffa5f97542ce3d027b19ee006

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

3a9e3b5Yet another docstring fix

comment:18 in reply to: ↑ 15 ; follow-up: Changed 3 years ago by jsrn

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:

3a9e3b5Yet another docstring fix

New commits:

3a9e3b5Yet another docstring fix

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by ncohen

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

Last edited 3 years ago by ncohen (previous) (diff)

comment:20 in reply to: ↑ 19 Changed 3 years ago by jsrn

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.

comment:21 Changed 3 years ago by git

  • Commit changed from 3a9e3b5cbb00db4ffa5f97542ce3d027b19ee006 to 62fae3d72ebc65be67fd72a426f744a581dda42d

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

62fae3dMore reviewer^2 comments

comment:22 Changed 3 years ago by git

  • Commit changed from 62fae3d72ebc65be67fd72a426f744a581dda42d to 37011a4187cbd3d91ca68224a82151666682dd67

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

37011a4trac #18926: Cleaning

comment:23 Changed 3 years ago by ncohen

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

comment:24 Changed 3 years ago by jsrn

  • Reviewers set to Johan Sebastian Rosenkilde Nielsen
  • Status changed from needs_review to positive_review

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

comment:25 Changed 3 years ago by ncohen

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

Nathann

comment:26 Changed 3 years ago by ncohen

And thank you very much for your help!!!

Nathann

comment:27 Changed 3 years ago by jsrn

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.

comment:28 Changed 3 years ago by vbraun

  • Branch changed from public/18926 to 37011a4187cbd3d91ca68224a82151666682dd67
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.