#28698 closed defect (fixed)

py3: HTML documentation for GlobalOptions does not show up correctly

Reported by: gh-mwageringel Owned by:
Priority: major Milestone: sage-9.0
Component: python3 Keywords:
Cc: klee Merged in:
Authors: Kwankyu Lee Reviewers: Markus Wageringel
Report Upstream: N/A Work issues:
Branch: 16b5e11 (Commits, GitHub, GitLab) Commit: 16b5e119b4db974e0f180c0fcd3f87d04022a446
Dependencies: Stopgaps:

Status badges

Description

With Python 3, the HTML documentation for GlobalOptions does not show the docstring, but something else. For example in the case of QQbar.options, this is what gets displayed:

options = Current options for AlgebraicField - display_format: decimal

This is what is called "String form" in the introspection QQbar.options?.

With Python 2, the HTML documentation shows the "Docstring" as desired:

options(*get_value, **set_value)
OPTIONS:

* "display_format" -- (default: "decimal")

  * "decimal" -- Always display a decimal approximation

  * "radical" -- Display using radicals (if possible)

See "GlobalOptions" for more features of these options.

This is not unique to QQbar, but can also be seen with Tableaux.options, for instance.

Change History (14)

comment:1 Changed 18 months ago by jhpalmieri

Can you provide more details? From the Sage command line or from the Jupyter notebook, if I type QQbar.options?, I see the same thing in Python 2 or Python 3.

comment:2 Changed 18 months ago by gh-mwageringel

It is displayed correctly on the command line. The problem is with the HTML files that get generated as part of the documentation. Currently, the problem can be seen here online: Python 2/Python 3.

comment:3 Changed 18 months ago by egourgoulhon

I confirm the bug in Sage 9.0.beta4 (Python 3 version). It holds for Manifold.options as well.

comment:4 follow-up: Changed 18 months ago by gh-mwageringel

The following (non-sensical) change in sage_autodoc.AttributeDocumenter.can_document_member appears to fix this problem.

  • src/sage_setup/docbuild/ext/sage_autodoc.py

    a b class AttributeDocumenter(DocstringStripSignatureMixin, ClassLevelDocumenter): 
    14771477
    14781478        isattribute = isdatadesc or (not isinstance(parent, ModuleDocumenter) and isattr)
    14791479
    14801480        # Trac #26522: This condition is here just to pass objects of classes
    14811481        # that inherit ClasscallMetaclass as attributes rather than method
    14821482        # descriptors.
    14831483        isattribute = isattribute or isinstance(type(member), ClasscallMetaclass)
    14841484
    1485         if PY2:
     1485        if PY2 or True:
    14861486            return isattribute
    14871487
    14881488        # That last condition addresses an obscure case of C-defined
    14891489        # methods using a deprecated type in Python 3, that is not otherwise
    14901490        # exported anywhere by Python
    14911491        return isattribute or (not isinstance(parent, ModuleDocumenter) and
    14921492                              not inspect.isroutine(member) and
    14931493                              not isinstance(member, type))

The problem is that sage_autodoc determines that QQbar.options should be documented as an attribute rather than a function, in Python 3, because the expression in the last return statement is true.

Note that class options(GlobalOptions) does not create a subclass, but an instance of GlobalOptions by some magic, so it acually is an attribute, which nevertheless should not be documented as such.

Also note that sage_autodoc is a modified copy of Sphinx' autodoc. The problematic expression has not been added by Sage, but is part of the upstream version. The PY2-check was added in #26949 when sage_autodoc was updated to be more in line with the upstream version (which is Python-3-only I assume).

Any ideas how to solve this? It would be good if we could avoid adding a special case for GlobalOptions and also avoid deviating from upstream too much.

comment:5 Changed 18 months ago by jhpalmieri

  • Cc klee added

@klee: any suggestions?

comment:6 Changed 18 months ago by klee

  • Authors set to Kwankyu Lee
  • Branch set to u/klee/28698

comment:7 Changed 18 months ago by git

  • Commit set to 16b5e119b4db974e0f180c0fcd3f87d04022a446

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

16b5e11Fix sage_auto_doc for GlobalOptions to appear correctly.

comment:8 in reply to: ↑ 4 Changed 18 months ago by klee

Replying to gh-mwageringel:

Any ideas how to solve this? It would be good if we could avoid adding a special case for GlobalOptions and also avoid deviating from upstream too much.

The problem is that GlobalOptions attribute get documented by AttributeDocumenter unlike by FunctionDocumenter in python 2. This is because of the extra check for "obscure case" in python 3.

I don't clearly understand what the "obscure case" is. Anyway, the extra check is apparently doing more harm than good to sage documentation. In the patch, I turned off that check, as you originally suggested. I expect the generated documentation is more close to the version in python 2.

We need to examine the generated documentation to see if there is any regression from the version in python 2.

Last edited 18 months ago by klee (previous) (diff)

comment:9 Changed 18 months ago by klee

  • Status changed from new to needs_review

comment:10 follow-up: Changed 18 months ago by gh-mwageringel

  • Status changed from needs_review to needs_info

I ran a diff on the generated html files before and after the patch using Python 3. The change mainly affects all the options classes that are now documented as function rather than attribute.

Besides options, there are two or three cases where the documentation also switched from attribute to function, for example

FiniteStateMachine.default_format_letter

which is just an alias to an imported function

    default_format_letter = latex

so default_format_letter now shows the documentation from latex, which seems correct.

However, this patch also affects a number of attributes that completely disappear from the documentation after the patch, for example:

AbstractGrowthGroupFunctor.rank, GenericGrowthGroup.CartesianProduct, GenericSymbolicSubringFunctor.coercion_reversed, CartesianProductFunctor.symbol, A000027.link, CNFEncoder.permutations

Are these supposed to appear in the documentation or not? Most of them do not have documentation strings attached to them, but some do (possibly unintentionally), for example:

FSMState.initial_probability

comment:11 in reply to: ↑ 10 Changed 18 months ago by klee

Replying to gh-mwageringel:

I ran a diff on the generated html files before and after the patch using Python 3.

Thanks.

The change mainly affects all the options classes that are now documented as function rather than attribute.

Good.

Besides options, there are two or three cases where the documentation also switched from attribute to function, for example

FiniteStateMachine.default_format_letter

which is just an alias to an imported function

    default_format_letter = latex

so default_format_letter now shows the documentation from latex, which seems correct.

Seems ok.

However, this patch also affects a number of attributes that completely disappear from the documentation after the patch, for example:

AbstractGrowthGroupFunctor.rank, GenericGrowthGroup.CartesianProduct, GenericSymbolicSubringFunctor.coercion_reversed, CartesianProductFunctor.symbol, A000027.link, CNFEncoder.permutations

Are these supposed to appear in the documentation or not?

They did not appear in python 2 documentation, but started to appear in python 3 documentation by #26949, perhaps because of the extra check in AttributeDocumenter. So I think they should not appear in the documentation. I regard this as an improvement.

Most of them do not have documentation strings attached to them, but some do (possibly unintentionally), for example:

FSMState.initial_probability

This example is interesting.

The attribute initial_probability did have docstring, but as of sage 9.0.beta4, the docstring is removed, or rather moved into the class (FSMState) docstring. This is why initial_probability (and is_initial) disappeared from the python 3 documentation.

Then why the docstring was moved to class docstring? Perhaps because the attribute docstrings do not get doctested. I think this is a defect of sage doctesting -- the original docstring was nice!

To summarize, I see no regression from your report.

comment:12 Changed 18 months ago by klee

  • Status changed from needs_info to needs_review

comment:13 Changed 18 months ago by gh-mwageringel

  • Reviewers set to Markus Wageringel
  • Status changed from needs_review to positive_review

Ok, thank you for the explanations. Considering that the documentation in Python 3 now seems to agree with the usual Python 2 documentation, this is good to go then.

comment:14 Changed 18 months ago by vbraun

  • Branch changed from u/klee/28698 to 16b5e119b4db974e0f180c0fcd3f87d04022a446
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.