Opened 5 years ago

Closed 4 years ago

#19482 closed enhancement (wontfix)

Documentation for auto-generated index of functions

Reported by: ncohen Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: documentation Keywords:
Cc: Merged in:
Authors: Nathann Cohen Reviewers: Jori Mäntysalo
Report Upstream: N/A Work issues:
Branch: u/ncohen/19482 (Commits) Commit: 757a9f4a908db4be170f28a96c000a1358ac9f2d
Dependencies: Stopgaps:

Description

This could have a place in the developer's manual, or it will be yet another dark feature.

Nathann

Change History (26)

comment:1 Changed 5 years ago by ncohen

  • Branch set to u/ncohen/19482
  • Commit set to 757a9f4a908db4be170f28a96c000a1358ac9f2d
  • Status changed from new to needs_review

New commits:

757a9f4trac #19482: Documentation for auto-generated index of functions

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

I tried to follow instructions with categories/finite_posets.py, but got only

docstring of sage.categories.finite_posets:11: WARNING: The "csv-table" directive requires content; none supplied.

I guess that I should change __name__ to something. To what? And should it be told in these instructions?

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

Yo,

I tried to follow instructions with categories/finite_posets.py, but got only

docstring of sage.categories.finite_posets:11: WARNING: The "csv-table" directive requires content; none supplied.

Would I be correct to guess that you followed the instructions for an index of *functions* in a file that does not define any?

In that file, I'd say that what you want are the methods of a specific class.

I guess that I should change __name__ to something.

Nono, this should work.

And should it be told in these instructions?

Let us first figure out what went wrong in your test.

Nathann

comment:4 in reply to: ↑ 3 Changed 5 years ago by jmantysalo

Replying to ncohen:

Would I be correct to guess that you followed the instructions for an index of *functions* in a file that does not define any?

There is is_lattice(), is_poset_isomorphism() etc. defined in categories/finite_posets.py.

comment:5 follow-up: Changed 5 years ago by ncohen

I just checked my terminology, but it seems correct: https://en.wikipedia.org/wiki/Method_(computer_programming)

A 'method' is a function defined in a class. The functions are the functions defined in a module.

sage: import inspect
sage: P = Poset()
sage: inspect.ismethod(P.is_lattice)
True
sage: inspect.isfunction(P.is_lattice)
False

Nathann

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

Replying to ncohen:

A 'method' is a function defined in a class. The functions are the functions defined in a module.

OK. Now I tried to read incidence_structures.py and copypaste it to finite_posets.py. It start with

r"""
Finite posets

List of functions:

{METHODS_OF_FinitePosets}
"""

and ends

from sage.misc.rest_index_of_methods import gen_rest_table_index
__doc__ = __doc__.format(METHODS_OF_FinitePosets=gen_rest_table_index(FinitePosets))

There might be no problem at all for clever people, but I do not understand.

comment:7 Changed 5 years ago by ncohen

That's because of the syntax of python's format thing:

sage: "a is {a} and b is {b}".format(a=4,b=5)
'a is 4 and b is 5'

Then you have to know that __doc__ is the name of the module's docstring (the string that starts with 'Finite posets'. And then gen_rest_table_index does its job from the class that you give it.

I don't mind your telling me that it is not clear or has to be explained in the developer's manual: it would be cool if we don't have to explain how python works there, though.

But then perhaps those details should not appear in the developer's manual but rather on the module's doc of this rest_index_of_methods module.

Nathann

comment:8 follow-up: Changed 5 years ago by vbraun

I guess the question is whether we really want widespread use of a hack that does large-ish string processing at each import (including during Sage startup).

An example for how to do things right is http://docs.astropy.org/en/stable/nddata/index.html, the auto-generated class table at the bottom is created only once while running sphinx to build the docs.

comment:9 in reply to: ↑ 8 Changed 5 years ago by ncohen

I guess the question is whether we really want widespread use of a hack that does large-ish string processing at each import (including during Sage startup).

Not at all. The question here is whether you want the interface of this code to be documented, so that people can use it.

How it is implemented is a different problem, and you are most welcome to help if you think that it should be written differently.

Nathann

comment:10 Changed 5 years ago by ncohen

What you say is true, however. The way they do it (through Sphinx) is much cleaner. I haven't found the list of method of a given classes so far, but it's probably doable with their code.

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

Back to this one...

I would not say "Optionally, the module's documentation can include a list - -", but something like "Module documentation should include a list of all functions. If there are only few of them, the list can be generated - -".

Maybe after example it could also say something like "The list will not contain functions that are deprecated or whose name start with underscore." I am not sure if this is good addition or just noise.

After "An index of all methods of a specific class - -" add "There is no automatic way to get a list of all functions defined in a module containing several classes."

"When a class/module defines many methods/functions it can become interesting - -" should be "When a class/module defines many methods/functions the list should be grouped thematically - -".

Otherwise I think that I have now understood how to use this and what can be done with automatic indexing.

comment:12 Changed 5 years ago by jmantysalo

  • Reviewers set to Jori Mäntysalo

comment:13 Changed 5 years ago by jmantysalo

  • Milestone changed from sage-6.10 to sage-7.0

comment:14 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by ncohen

Yo !

I would not say "Optionally, the module's documentation can include a list - -", but something like "Module documentation should include a list of all functions. If there are only few of them, the list can be generated - -".

HMmmm.. Sorry for nitpicking, but I don't allow myself to write rules in our developer's manual without a poll on sage-devel. Around 0% of our modules follow this rule at the moment, and phrasing it like that might say that we could refuse branches because they don't have it.

Maybe after example it could also say something like "The list will not contain functions that are deprecated or whose name start with underscore." I am not sure if this is good addition or just noise.

Well, to me this is more documentation specific to the reference manual. There is a link toward the rest_index_of_methods module that people can follow, and see what is there. So what you say is necessary <somewhere> but I don't personall believe it must be here.

After "An index of all methods of a specific class - -" add "There is no automatic way to get a list of all functions defined in a module containing several classes."

Errr... This sentence is 'wrong', technically. A function, by definition, does not belong to a class. A method, by definition, belongs to a class. You want to say that you cannot have one big table that mixes together the methods of different classes? That's rather expected, isn't it?.. :-P

"When a class/module defines many methods/functions it can become interesting - -" should be "When a class/module defines many methods/functions the list should be grouped thematically - -".

Sorry, same reason. I don't want to use 'should' here. I don't mind making tools available, though I don't want to force people to use them. Especially when some don't like them.

Nathann

comment:15 Changed 5 years ago by vbraun

Warning: this slows down the module import and should at most be used for modules that are lazily imported.

comment:16 Changed 5 years ago by ncohen

See? "Can be used". Not "should be used" :-P

comment:17 in reply to: ↑ 14 ; follow-up: Changed 5 years ago by jmantysalo

In any case, this is better with this addition than without, so I mark this as positive review. But some comments:

[ By comment of vbraun in time of writing this I cancelled positive review. ]

Replying to ncohen:

After "An index of all methods of a specific class - -" add "There is no automatic way to get a list of all functions defined in a module containing several classes."

Errr... This sentence is 'wrong', technically. A function, by definition, does not belong to a class. A method, by definition, belongs to a class. You want to say that you cannot have one big table that mixes together the methods of different classes? That's rather expected, isn't it?.. :-P

Well, it is not that strange to think that functions, methods, classes, and whatevers, defined in one file can be shown in one index.

I would not say "Optionally, the module's documentation can include a list - -", but something like "Module documentation should include a list of all functions. If there are only few of them, the list can be generated - -".

HMmmm.. Sorry for nitpicking, but I don't allow myself to write rules in our developer's manual without a poll on sage-devel. Around 0% of our modules follow this rule at the moment, and phrasing it like that might say that we could refuse branches because they don't have it.

Actually I would like to have a kind of rule that new modules must have an index. Not necessarily made with this, but somehow.

But this has been discussed before without result. You want only rules that are really rules; I would like to have also recommendations of the form "Do X or say why you did not do."

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

Well, it is not that strange to think that functions, methods, classes, and whatevers, defined in one file can be shown in one index.

What do you do when several classes have methods with the same name?

Actually I would like to have a kind of rule that new modules must have an index. Not necessarily made with this, but somehow.

But this has been discussed before without result. You want only rules that are really rules; I would like to have also recommendations of the form "Do X or say why you did not do."

Hmmmm... Well, then add a commit if you like?

Nathann

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

Replying to ncohen:

Well, it is not that strange to think that functions, methods, classes, and whatevers, defined in one file can be shown in one index.

What do you do when several classes have methods with the same name?

Have a link with class name added. ... ok, ok, bad idea.

Hmmmm... Well, then add a commit if you like?

Maybe not for this one.

But anyways, should this be accepted or not? Volker, is a warning needed in this place, in the documentation of gen_rest...(), or in both? Or not at all?

comment:20 in reply to: ↑ 19 Changed 5 years ago by jmantysalo

Replying to jmantysalo:

But anyways, should this be accepted or not? Volker, is a warning needed in this place, in the documentation of gen_rest...(), or in both? Or not at all?

Just pinging... What should we do with this?

comment:21 Changed 5 years ago by ncohen

(test request from Jori. Please ignore)

comment:22 Changed 5 years ago by jmantysalo

Getting back to this one... Volker, I still think that Sage with this patch is better than Sage without. As Nathann said, without this we have another dark feature.

comment:23 follow-up: Changed 5 years ago by vbraun

Startup time for Sage is also an issue, we are basically already borderline too slow to start. Doing more stuff at startup is not going to help. Related, the proposed syntax of slapping some magic stuff at the bottom of each page isn't something that you should do if you had a proper implementation. If you do it right (did you look at the astropy example?) then you wouldn't need the magic __doc__ = ... anyways. IMHO we should have a good implementation first before encouraging others to use it.

comment:24 in reply to: ↑ 23 Changed 5 years ago by jmantysalo

Replying to vbraun:

IMHO we should have a good implementation first before encouraging others to use it.

OK. And I think that we all agree that Sphinx gives headache to everyone.

There is now five files that uses this (2 in combinat, 2 in coding, 1 in graphs). Should we add some comment to those, just before {INDEX_OF_FUNCTIONS} to tell developers what is going on?

comment:25 Changed 5 years ago by jmantysalo

  • Milestone changed from sage-7.0 to sage-duplicate/invalid/wontfix
  • Status changed from needs_review to positive_review

As there does not seem to be common view, and Nathann has faded from developers (sad), I mark this as wontfix/positive. Volker, please revert if you have other opinion.

comment:26 Changed 4 years ago by embray

  • Resolution set to wontfix
  • Status changed from positive_review to closed

Determined to be invalid/duplicate/wontfix (closing as "wontfix" as a catch-all resolution).

Note: See TracTickets for help on using tickets.