Opened 6 years ago
Closed 6 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 6 years ago by
- Branch set to public/18926
- Commit set to b23189624b8e9c913af0140fb0a036291340ce8b
- Status changed from new to needs_review
comment:3 Changed 6 years ago by
- Cc dlucas added
comment:4 Changed 6 years ago by
- Commit changed from b23189624b8e9c913af0140fb0a036291340ce8b to e1a3241375269c7f8bff7431e5bceee79f094641
Branch pushed to git repo; I updated commit sha1. New commits:
e1a3241 | trac #18926: Table from #18636
|
comment:5 follow-up: ↓ 6 Changed 6 years ago by
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 6 years ago by
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: ↓ 9 Changed 6 years ago by
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__
, thendir(root)
would find everything and his grandmother. Could that be more clever? Here the checkinspect.getmodule(root) == ...
also seems to get in the way for listing class methods.
Johan
comment:8 Changed 6 years ago by
- Status changed from needs_review to needs_info
comment:9 in reply to: ↑ 7 Changed 6 years ago by
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__
, thendir(root)
would find everything and his grandmother. Could that be more clever? Here the checkinspect.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 6 years ago by
- Commit changed from e1a3241375269c7f8bff7431e5bceee79f094641 to eb276db9b51097b202cc9a5e1f71f1f9ca599925
comment:11 Changed 6 years ago by
- Status changed from needs_info to needs_review
comment:12 Changed 6 years ago by
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 6 years ago by
- Commit changed from eb276db9b51097b202cc9a5e1f71f1f9ca599925 to 5fc4e9e9bffe705a66386bce27655da00727bd1a
comment:14 Changed 6 years ago by
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: ↓ 18 Changed 6 years ago by
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 includinggen_rest_table_index
in the index of ... its own module. Why wouldn'tonly_local=False
print always more thanonly_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 6 years ago by
- Commit changed from 5fc4e9e9bffe705a66386bce27655da00727bd1a to 9325bb36ef1e6d0a16a986c7a6ecf20c104fef40
Branch pushed to git repo; I updated commit sha1. New commits:
9325bb3 | Reviewer^2 fixes
|
comment:17 Changed 6 years ago by
- Commit changed from 9325bb36ef1e6d0a16a986c7a6ecf20c104fef40 to 3a9e3b5cbb00db4ffa5f97542ce3d027b19ee006
Branch pushed to git repo; I updated commit sha1. New commits:
3a9e3b5 | Yet another docstring fix
|
comment:18 in reply to: ↑ 15 ; follow-up: ↓ 19 Changed 6 years ago by
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 includinggen_rest_table_index
in the index of ... its own module. Why wouldn'tonly_local=False
print always more thanonly_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
|
comment:19 in reply to: ↑ 18 ; follow-up: ↓ 20 Changed 6 years ago by
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 withonly_local=False
: you want all the non-local functions, exceptgen_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
comment:20 in reply to: ↑ 19 Changed 6 years ago by
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 listO_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 6 years ago by
- Commit changed from 3a9e3b5cbb00db4ffa5f97542ce3d027b19ee006 to 62fae3d72ebc65be67fd72a426f744a581dda42d
Branch pushed to git repo; I updated commit sha1. New commits:
62fae3d | More reviewer^2 comments
|
comment:22 Changed 6 years ago by
- Commit changed from 62fae3d72ebc65be67fd72a426f744a581dda42d to 37011a4187cbd3d91ca68224a82151666682dd67
Branch pushed to git repo; I updated commit sha1. New commits:
37011a4 | trac #18926: Cleaning
|
comment:23 Changed 6 years ago by
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 6 years ago by
- 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 6 years ago by
Exxxxxxxxxcellent. Thanks. We now have free index of functions, nothing to do by hand. Cool.
Nathann
comment:26 Changed 6 years ago by
And thank you very much for your help!!!
Nathann
comment:27 Changed 6 years ago by
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 6 years ago by
- Branch changed from public/18926 to 37011a4187cbd3d91ca68224a82151666682dd67
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
trac #18926: Auto-generated index of functions