Opened 8 months ago

Closed 4 months ago

#27692 closed task (fixed)

py3: fix src/sage/misc/nested_class.pyx

Reported by: chapoton Owned by: embray
Priority: major Milestone: sage-8.9
Component: python3 Keywords:
Cc: embray, jdemeyer, fbissey Merged in:
Authors: Erik Bray, Kwankyu Lee Reviewers: Frédéric Chapoton, John Palmieri
Report Upstream: N/A Work issues:
Branch: 562ff1e (Commits) Commit: 562ff1e1f69908bdc5a622a61510d5bdea8c51b2
Dependencies: Stopgaps:

Description

traceback:

sage -t --long src/sage/misc/nested_class.pyx
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 39, in sage.misc.nested_class
Failed example:
    A1.A2.A3
Expected:
    <class sage.misc.nested_class.A3 at ...>
Got:
    <class 'sage.misc.nested_class.A1.A2.A3'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 42, in sage.misc.nested_class
Failed example:
    nested_pickle(A1)
Expected:
    <class sage.misc.nested_class.A1 at ...>
Got:
    <class 'sage.misc.nested_class.A1'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 45, in sage.misc.nested_class
Failed example:
    A1.A2
Expected:
    <class sage.misc.nested_class.A1.A2 at ...>
Got:
    <class 'sage.misc.nested_class.A1.A2'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 48, in sage.misc.nested_class
Failed example:
    A1.A2.A3
Expected:
    <class sage.misc.nested_class.A1.A2.A3 at ...>
Got:
    <class 'sage.misc.nested_class.A1.A2.A3'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 131, in sage.misc.nested_class.modify_for_nested_pickle
Failed example:
    getattr(module, 'A.B', 'Not found')
Expected:
    <class '__main__.X.A.B'>
Got:
    <class '__main__.A.B'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 133, in sage.misc.nested_class.modify_for_nested_pickle
Failed example:
    getattr(module, 'X.A.B', 'Not found')
Expected:
    <class '__main__.X.A.B'>
Got:
    <class '__main__.A.B'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 219, in sage.misc.nested_class.nested_pickle
Failed example:
    getattr(module, 'A.B', 'Not found')
Expected:
    <class __main__.A.B at ...>
Got:
    <class '__main__.A.B'>
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 259, in sage.misc.nested_class.NestedClassMetaclass
Failed example:
    A3.B.__name__
Expected:
    'A3.B'
Got:
    'B'
**********************************************************************
File "src/sage/misc/nested_class.pyx", line 261, in sage.misc.nested_class.NestedClassMetaclass
Failed example:
    getattr(sys.modules['__main__'], 'A3.B', 'Not found')
Expected:
    <class '__main__.A3.B'>
Got:
    'Not found'
**********************************************************************
4 items had failures:
   4 of  14 in sage.misc.nested_class
   2 of   6 in sage.misc.nested_class.NestedClassMetaclass
   2 of  23 in sage.misc.nested_class.modify_for_nested_pickle
   1 of   9 in sage.misc.nested_class.nested_pickle
    [68 tests, 9 failures, 1.77 s]

Change History (92)

comment:1 Changed 8 months ago by nbruin

To get the same repr for a class (without "at ...") in both Py2 and Py3, it may be as simple as defining the classes with class A(object):. It looks like the difference in printing is between old-style and new-style classes, and python3 has only new style classes. So, including the (object) brings the example in Py2 closer to what it does in Py3.

The problem with "name" might need more work.

comment:2 Changed 8 months ago by embray

At some point I definitely fixed or changed something related to this but I'm wracking my brain to remember where.

comment:3 Changed 8 months ago by embray

IIRC one of the main issues here is that I (?) fixed pickling of nested classes such that NestedClassMetaclass is all but unnecessary. But I might have dreamed it; it's all a bit foggy.

comment:4 Changed 8 months ago by embray

Here it is--it turns out that the issue this is trying to work around just isn't relevant on Python 3, so it makes NestedClassMetaclass a no-op to be removed whenever we drop Python 2 support.

  • src/sage/misc/nested_class.pyx

    diff --git a/src/sage/misc/nested_class.pyx b/src/sage/misc/nested_class.pyx
    index 3cd3dfc..cf9c352 100644
    a b EXAMPLES:: 
    3636
    3737    sage: A1.A2.A3.__name__
    3838    'A3'
    39     sage: A1.A2.A3
     39    sage: A1.A2.A3  # py2
    4040    <class sage.misc.nested_class.A3 at ...>
    4141
    42     sage: nested_pickle(A1)
     42    sage: nested_pickle(A1)  # py2
    4343    <class sage.misc.nested_class.A1 at ...>
    4444
    45     sage: A1.A2
     45    sage: A1.A2  # py2
    4646    <class sage.misc.nested_class.A1.A2 at ...>
     47    sage: A1.A2  # py3 - note: here we have he desired name for free
     48    <class 'sage.misc.nested_class.A1.A2'>
    4749
    48     sage: A1.A2.A3
     50    sage: A1.A2.A3  # py2
    4951    <class sage.misc.nested_class.A1.A2.A3 at ...>
    50     sage: A1.A2.A3.__name__
     52    sage: A1.A2.A3  # py3
     53    <class 'sage.misc.nested_class.A1.A2.A3'>
     54    sage: A1.A2.A3.__name__  # py2
    5155    'A1.A2.A3'
    5256
    53     sage: sage.misc.nested_class.__dict__['A1.A2'] is A1.A2
     57    sage: sage.misc.nested_class.__dict__['A1.A2'] is A1.A2  # py2
    5458    True
    55     sage: sage.misc.nested_class.__dict__['A1.A2.A3'] is A1.A2.A3
     59    sage: sage.misc.nested_class.__dict__['A1.A2.A3'] is A1.A2.A3  # py2
    5660    True
    5761
    5862All of this is not perfect. In the following scenario::
    cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True): 
    8589    giving them a mangled name and putting the mangled name in the
    8690    module namespace.
    8791
     92    On Python 3 this is a no-op and does not perform any mangling.
     93
    8894    INPUT:
    8995
    9096    - ``cls`` - The class to modify.
    cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True): 
    111117        sage: getattr(module, 'A.B', 'Not found')
    112118        'Not found'
    113119        sage: modify_for_nested_pickle(A, 'A', sys.modules['__main__'])
    114         sage: A.B.__name__
     120        sage: A.B.__name__  # py2
    115121        'A.B'
    116         sage: getattr(module, 'A.B', 'Not found')
     122        sage: getattr(module, 'A.B', 'Not found')  # py2
    117123        <class '__main__.A.B'>
    118124
    119125    Here we demonstrate the effect of the ``first_run`` argument::
    120126
    121127        sage: modify_for_nested_pickle(A, 'X', sys.modules['__main__'])
    122         sage: A.B.__name__ # nothing changed
     128        sage: A.B.__name__ # py2: nothing changed
    123129        'A.B'
    124130        sage: modify_for_nested_pickle(A, 'X', sys.modules['__main__'], first_run=False)
    125         sage: A.B.__name__
     131        sage: A.B.__name__  # py2
    126132        'X.A.B'
    127133
    128134    Note that the class is now found in the module under both its old and
    129135    its new name::
    130136
    131         sage: getattr(module, 'A.B', 'Not found')
     137        sage: getattr(module, 'A.B', 'Not found')  # py2
    132138        <class '__main__.X.A.B'>
    133         sage: getattr(module, 'X.A.B', 'Not found')
     139        sage: getattr(module, 'X.A.B', 'Not found')  # py2
    134140        <class '__main__.X.A.B'>
    135141
    136142
    cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True): 
    151157
    152158    Before :trac:`9107`, the name of ``A1.B1.C1`` would have been wrong::
    153159
    154         sage: A1.B1.C1.__name__
     160        sage: A1.B1.C1.__name__  # py2
    155161        'A1.B1.C1'
    156         sage: A1.B2.C2.__name__
     162        sage: A1.B2.C2.__name__  # py2
    157163        'A1.B2.C2'
    158164        sage: A_module = sys.modules[A1.__module__]
    159         sage: getattr(A_module, 'A1.B1.C1', 'Not found').__name__
     165        sage: getattr(A_module, 'A1.B1.C1', 'Not found').__name__  # py2
    160166        'A1.B1.C1'
    161         sage: getattr(A_module, 'A1.B2.C2', 'Not found').__name__
     167        sage: getattr(A_module, 'A1.B2.C2', 'Not found').__name__  # py2
    162168        'A1.B2.C2'
    163169
    164170    """
     171    IF PY_MAJOR_VERSION >= 3:
     172        return
     173
    165174    cdef str name, dotted_name
    166175    cdef str mod_name = module.__name__
    167176    cdef str cls_name = cls.__name__+'.'
    def nested_pickle(cls): 
    214223    then the name of class ``"B"`` will be modified to ``"A.B"``, and the ``"A.B"``
    215224    attribute of the module will be set to class ``"B"``::
    216225
    217         sage: A.B.__name__
     226        sage: A.B.__name__  # py2
    218227        'A.B'
    219         sage: getattr(module, 'A.B', 'Not found')
     228        sage: getattr(module, 'A.B', 'Not found')  # py2
    220229        <class __main__.A.B at ...>
    221230
    222231    In Python 2.6, decorators work with classes; then ``@nested_pickle``
    cdef class NestedClassMetaclass(type): 
    245254    r"""
    246255    A metaclass for nested pickling.
    247256
     257    On Python 3 this is just a direct wrapper around the base `type` and does
     258    not do anything.
     259
    248260    Check that one can use a metaclass to ensure nested_pickle
    249261    is called on any derived subclass::
    250262
    cdef class NestedClassMetaclass(type): 
    256268        ....:     class B(object):
    257269        ....:         pass
    258270        ...
    259         sage: A3.B.__name__
     271        sage: A3.B.__name__  # py2
    260272        'A3.B'
    261         sage: getattr(sys.modules['__main__'], 'A3.B', 'Not found')
     273        sage: getattr(sys.modules['__main__'], 'A3.B', 'Not found')  # py2
    262274        <class '__main__.A3.B'>
    263275    """
     276
    264277    def __init__(self, *args):
    265278        r"""
    266279        This invokes the nested_pickle on construction.
    cdef class NestedClassMetaclass(type): 
    273286        ...
    274287        sage: A.B
    275288        <class '__main__.A.B'>
    276         sage: getattr(sys.modules['__main__'], 'A.B', 'Not found')
     289        sage: getattr(sys.modules['__main__'], 'A.B', 'Not found')  # py2
    277290        <class '__main__.A.B'>
    278291        """
    279292        modify_for_nested_pickle(self, self.__name__, sys_modules[self.__module__])
    class MainClass(object, metaclass=NestedClassMetaclass): 
    306319                sage: from sage.misc.nested_class import *
    307320                sage: loads(dumps(MainClass.NestedClass.NestedSubClass()))
    308321                <sage.misc.nested_class.MainClass.NestedClass.NestedSubClass object at 0x...>
    309                 sage: getattr(sage.misc.nested_class, 'MainClass.NestedClass.NestedSubClass')
     322                sage: getattr(sage.misc.nested_class, 'MainClass.NestedClass.NestedSubClass')  # py2
    310323                <class 'sage.misc.nested_class.MainClass.NestedClass.NestedSubClass'>
    311                 sage: MainClass.NestedClass.NestedSubClass.__name__
     324                sage: MainClass.NestedClass.NestedSubClass.__name__  # py2
    312325                'MainClass.NestedClass.NestedSubClass'
    313326            """
    314327            def dummy(self, x, *args, r=(1,2,3.4), **kwds):
    class MainClass(object, metaclass=NestedClassMetaclass): 
    321334                    sage: from sage.misc.nested_class import MainClass
    322335                    sage: print(MainClass.NestedClass.NestedSubClass.dummy.__doc__)
    323336                    NestedSubClass.dummy(self, x, *args, r=(1, 2, 3.4), **kwds)
    324                     File: sage/misc/nested_class.pyx (starting at line 314)
     337                    File: sage/misc/nested_class.pyx (starting at line 327)
    325338                    <BLANKLINE>
    326339                                    A dummy method to demonstrate the embedding of
    327340                                    method signature for nested classes.

comment:5 Changed 8 months ago by embray

And most of the tests are skipped on Python 3 as they are not relevant.

comment:6 Changed 8 months ago by chapoton

branch please

comment:7 Changed 8 months ago by embray

  • Authors set to Erik Bray
  • Branch set to u/embray/python3/ticket-27692
  • Commit set to c876038bf90c23efcb0e95e2757898f3358cec9b
  • Status changed from new to needs_review

Here you go. With this, all the tests for src/sage/misc/nested_class.pyx pass on Python 3 (though many of them are simply skipped as inapplicable).

There might be some other minor failures related to this in miscellaneous modules, but I am not sure. Everything in python3-known-passing.txt still passes.


New commits:

c876038py3: Make NestedClassMetaclass do nothing on Python 3

comment:8 Changed 8 months ago by chapoton

one failing doctest in src/sage/misc/sageinspect.py, see patchbot

comment:9 Changed 8 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:10 Changed 8 months ago by chapoton

  • Branch changed from u/embray/python3/ticket-27692 to public/python3/ticket-27692
  • Commit changed from c876038bf90c23efcb0e95e2757898f3358cec9b to a902cb23d6a81d04c49b9c0ceb5eab97d4f4a6c4
  • Status changed from needs_work to needs_review

I fixed the doctest


New commits:

a4e9eb3Merge branch 'u/embray/python3/ticket-27692' in 8.8.b3
a902cb2fix doctest in sageinspect

comment:11 Changed 8 months ago by embray

Right...there's always one of those...

comment:12 Changed 8 months ago by embray

  • Branch changed from public/python3/ticket-27692 to u/embray/python3/ticket-27692
  • Commit changed from a902cb23d6a81d04c49b9c0ceb5eab97d4f4a6c4 to 174e8956aa38a325ec189f786c4ee27c246c55fa
  • Status changed from needs_review to positive_review

New commits:

174e895py3: Make NestedClassMetaclass do nothing on Python 3

comment:13 Changed 8 months ago by chapoton

  • Reviewers set to Frédéric Chapoton

ok, great. Thx

comment:14 Changed 7 months ago by vbraun

  • Branch changed from u/embray/python3/ticket-27692 to 174e8956aa38a325ec189f786c4ee27c246c55fa
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:15 Changed 7 months ago by vbraun

  • Branch changed from 174e8956aa38a325ec189f786c4ee27c246c55fa to u/embray/python3/ticket-27692
  • Resolution fixed deleted
  • Status changed from closed to new

This ticket breaks docbuild on python3 (only); I don't know why but it does. Error is:

$ make doc-clean && make -j40 && make doc-html
[...]
[dochtml] Error building the documentation.
[dochtml] multiprocessing.pool.RemoteTraceback: 
[dochtml] """
[dochtml] Traceback (most recent call last):
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/multiprocessing/pool.py", line 121, in worker
[dochtml]     result = (True, func(*args, **kwds))
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/multiprocessing/pool.py", line 44, in mapstar
[dochtml]     return list(map(*args))
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/site-packages/sage_setup/docbuild/__init__.py", line 79, in build_ref_doc
[dochtml]     getattr(ReferenceSubBuilder(doc, lang), format)(*args, **kwds)
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/site-packages/sage_setup/docbuild/__init__.py", line 761, in _wrapper
[dochtml]     getattr(DocBuilder, build_type)(self, *args, **kwds)
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/site-packages/sage_setup/docbuild/__init__.py", line 133, in f
[dochtml]     runsphinx()
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 314, in runsphinx
[dochtml]     sys.stderr.raise_errors()
[dochtml]   File "/var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/site-packages/sage_setup/docbuild/sphinxbuild.py", line 249, in raise_errors
[dochtml]     raise OSError(self._error)
[dochtml] OSError: /var/lib/buildbot/slave/sage3_git/build/local/lib/python3.7/site-packages/sage/combinat/ncsf_qsym/qsym.py:docstring of sage.combinat.ncsf_qsym.qsym.QuasiSymmetricFunctions.Fundamental.Eulerian:55: WARNING: Duplicate explicit target name: "sw2010".

New commits:

174e895py3: Make NestedClassMetaclass do nothing on Python 3

comment:16 Changed 7 months ago by embray

  • Owner changed from (none) to embray

comment:17 Changed 7 months ago by embray

  • Status changed from new to needs_info

comment:18 Changed 7 months ago by embray

  • Status changed from needs_info to needs_work

Not sure why I can't just progress straight to needs_work...

comment:19 Changed 7 months ago by embray

I suppose it's possible there's some strange interaction between this, and how Sphinx treats nested classes (either in plain Sphinx, or something particular that Sage does with it...)

comment:20 Changed 7 months ago by jhpalmieri

Or how Sphinx treats cached methods? A bandaid (untested) might be to move the citation out of qsym.py to the master bibliography file.

comment:21 Changed 7 months ago by jhpalmieri

In fact that file has its own bibliography at the start, so this change might fix it:

  • src/sage/combinat/ncsf_qsym/qsym.py

    diff --git a/src/sage/combinat/ncsf_qsym/qsym.py b/src/sage/combinat/ncsf_qsym/qsym.py
    index ae7d830ed5..61004f24cd 100644
    a b REFERENCES: 
    6262   Quasisymmetric Functions Expand Positively into Young Quasisymmetric
    6363   Schur Functions*. :arxiv:`1606.03519`
    6464
     65.. [SW2010] John Shareshian and Michelle Wachs.
     66   *Eulerian quasisymmetric functions*. (2010).
     67   :arxiv:`0812.0764v2`
     68
    6569AUTHOR:
    6670
    6771- Jason Bandlow
    class QuasiSymmetricFunctions(UniqueRepresentation, Parent): 
    23482352            - ``k`` -- (optional) if specified, determines the number of fixed
    23492353              points of the permutations which are being summed over
    23502354
    2351             REFERENCES:
    2352 
    2353             .. [SW2010] John Shareshian and Michelle Wachs.
    2354                *Eulerian quasisymmetric functions*. (2010).
    2355                :arxiv:`0812.0764v2`
    2356 
    23572355            EXAMPLES::
    23582356
    23592357                sage: F = QuasiSymmetricFunctions(QQ).F()

Again, this is just a bandaid.

comment:22 Changed 7 months ago by fbissey

Good job on Volker to find it out. I couldn't figure which ticket was causing that breakage.

comment:23 Changed 7 months ago by embray

Hasn't there been some effort I've seen elsewhere to move bibliographies to module top-levels anyways (probably in part to prevent problems like this)?

comment:24 Changed 7 months ago by embray

Or, I guess maybe I'm thinking of #27497, which is not exactly as I described.

comment:25 Changed 7 months ago by jhpalmieri

The preferred thing is to move all references to the master bibliography file. combinat and graphs are the two directories remaining where this needs to be done.

comment:26 follow-up: Changed 7 months ago by jhpalmieri

I think my proposed change in comment:21 fixes the docbuilding problem. Should we incorporate it since it's the right thing to do to collect all of the references, or is it indicative of a further problem that needs investigating? (Do we care how Sphinx handles nested and/or cached methods, if that is indeed the problem?)

comment:27 in reply to: ↑ 26 Changed 7 months ago by embray

Replying to jhpalmieri:

I think my proposed change in comment:21 fixes the docbuilding problem. Should we incorporate it since it's the right thing to do to collect all of the references, or is it indicative of a further problem that needs investigating? (Do we care how Sphinx handles nested and/or cached methods, if that is indeed the problem?)

On one hand, it might be indicative of a deeper problem; on the other hand I don't think it's something I want to place high priority on. I agree that this change fixes the problem for now. I just want to confirm that the compiled docs do still look as expected and then I'll update this branch and set it back to positive review.

comment:28 follow-up: Changed 7 months ago by embray

There does appear to be a legitimate issue here w.r.t. Sphinx.

In the class QuasiSymmetricFunctions the nested class Fundamental also has an alias of just F. That is, in the class body it has:

F = Fundamental

just as a shortcut. This does not affect the qualified name of the class, which is still correct:

sage: QuasiSymmetricFunctions.Fundamental.__qualname__
'QuasiSymmetricFunctions.Fundamental'

but for some reason Sphinx is outputting the docs for QuasiSymmetricFunctions with this nested class listed as just "F" instead of "Fundamental", whereas in the current docs it's listed correctly as "Fundamental".

comment:29 Changed 6 months ago by embray

Back when I was last looking into this I was on the verge of tracking down the issue, but then everything went up in the air for a few weeks and now I'm not exactly sure where I was with this, though the issue did seem to be somewhere in one of Sage's Sphinx extensions.

comment:30 Changed 6 months ago by chapoton

Is there any hope for progress here ? This is now the top-failing file, by number of failures.

Last edited 6 months ago by chapoton (previous) (diff)

comment:31 Changed 6 months ago by embray

I think so. I (or someone) needs to work out exactly what's going on, IIRC in sage_autodoc, such that aliases for some nested classes are being preferred over the class itself.

I think it might just be some dict sorting issue, but it needs to actually explicitly detect cases where an object's attribute name is not the same as its __name__.

That is, when looping over the members of, for example, QuasiSymmetricFunctions, you'll find a Fundamental member and an F member which both point to the same object, the class named Fundamental. The docbuild is failing because this member is being listed as just "class F" and not "class Fundamental".

comment:32 Changed 6 months ago by git

  • Commit changed from 174e8956aa38a325ec189f786c4ee27c246c55fa to 42140750b92b7827d3619d0c3c6abbd9b2790292

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

4214075py3: Make NestedClassMetaclass do nothing on Python 3

comment:33 Changed 6 months ago by embray

Rebased. Going to keep investigating the docbuild issue now.

comment:34 Changed 6 months ago by git

  • Commit changed from 42140750b92b7827d3619d0c3c6abbd9b2790292 to 291d58442794d76d904404d9b58245d98d53ed18

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

291d584Move this reference to the module level.

comment:35 Changed 6 months ago by git

  • Commit changed from 291d58442794d76d904404d9b58245d98d53ed18 to 8337e67599e7e3e96c705fd48fd102123ace9dc4

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

8337e67Fix up sage_autodoc to work with normal Python 3 nested classes with qualnames,

comment:36 Changed 6 months ago by embray

The above commit fixes the docbuild issue for me. The resulting docs now look as expected: QuasiSymmetricFunctions.Fundamental is documented as a nested class in QuasiSymmetricFunctions, while QuasiSymmetricFunctions.F is documented as an attribute, which is simply an alias for QuasiSymmetricFunctions.Fundamental.

Need to double-check that this still holds on Python 2 with this branch.

comment:37 Changed 6 months ago by embray

  • Status changed from needs_work to needs_review

I think this is good for Python 2 as well. Strangely, the .F alias does not get documented in the autodoc for the class on Python 2, and that appears to be the case in the current version of the docs as well. That difference is consistent before and after this change, whereas the Python 3 version seems better so I'm content to leave it at that.

comment:38 Changed 6 months ago by jhpalmieri

This looks okay to me, but others should investigate, too.

comment:39 Changed 6 months ago by chapoton

patchbot reports some pyflakes errors (easy to fix) and a docbuild failure:

+[dochtml] OSError: /home/sage-patchbot/sage/local/lib/python2.7/site-packages/sage/combinat/dyck_word.py:
docstring of sage.combinat.dyck_word.DyckWord_complete.pyramid_weight:23:
WARNING: Duplicate explicit target name: "ds1992".

comment:40 follow-up: Changed 6 months ago by jhpalmieri

After I fix the references in dyck_word.py, I get a ton of other "Duplicate citation" and "Duplicate explicit target names" from other files in sage/combinat. For example:

[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/fully_packed_loop.py:docstring of sage.combinat.fully_packed_loop.FullyPackedLoops.Element:376: WARNING: Duplicate explicit target name: "propp2001".
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/fully_packed_loop.py:docstring of sage.combinat.fully_packed_loop.FullyPackedLoops.Element:381: WARNING: Duplicate explicit target name: "striker2015".
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/fully_packed_loop.py:docstring of sage.combinat.fully_packed_loop.FullyPackedLoops.Element.gyration:9: WARNING: Duplicate explicit target name: "wieland00".
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/fully_packed_loop.py:docstring of sage.combinat.fully_packed_loop.FullyPackedLoops.Element:376: WARNING: duplicate citation Propp2001, other instance in /Users/palmieri/Desktop/Sage_stuff/git/sage/src/doc/en/reference/combinat/sage/combinat/fully_packed_loop.rst
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/fully_packed_loop.py:docstring of sage.combinat.fully_packed_loop.FullyPackedLoops.Element:381: WARNING: duplicate citation Striker2015, other instance in /Users/palmieri/Desktop/Sage_stuff/git/sage/src/doc/en/reference/combinat/sage/combinat/fully_packed_loop.rst
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/fully_packed_loop.py:docstring of sage.combinat.fully_packed_loop.FullyPackedLoops.Element.gyration:9: WARNING: duplicate citation Wieland00, other instance in /Users/palmieri/Desktop/Sage_stuff/git/sage/src/doc/en/reference/combinat/sage/combinat/fully_packed_loop.rst
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/interval_posets.py:docstring of sage.combinat.interval_posets.TamariIntervalPosets_all.Element.cubical_coordinates:14: WARNING: Duplicate explicit target name: "combe2019".
[combinat ] /Users/palmieri/Desktop/Sage_stuff/git/sage/local/lib/python2.7/site-packages/sage/combinat/interval_posets.py:docstring of sage.combinat.interval_posets.TamariIntervalPosets_all.Element.cubical_coordinates:14: WARNING: duplicate citation Combe2019, other instance in /Users/palmieri/Desktop/Sage_stuff/git/sage/src/doc/en/reference/combinat/sage/combinat/interval_posets.rst

comment:41 Changed 6 months ago by jhpalmieri

This fixes the pyflakes errors:

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

    diff --git a/src/sage_setup/docbuild/ext/sage_autodoc.py b/src/sage_setup/docbuild/ext/sage_autodoc.py
    index 9cce254a13..b75791de36 100644
    a b AUTHORS: 
    3030import inspect
    3131import re
    3232import sys
    33 import warnings
    3433
    3534from docutils.statemachine import ViewList
    36 from six import PY2, iteritems, itervalues, text_type, class_types, string_types
     35from six import PY2, itervalues, text_type, class_types, string_types
    3736
    3837import sphinx
    39 from sphinx.errors import ExtensionError
    4038from sphinx.ext.autodoc.importer import mock, import_object, get_object_members
    41 from sphinx.ext.autodoc.inspector import format_annotation
    4239from sphinx.locale import _, __
    4340from sphinx.pycode import ModuleAnalyzer
    44 from sphinx.errors import ExtensionError, PycodeError
     41from sphinx.errors import PycodeError
    4542from sphinx.util import logging
    4643from sphinx.util import rpartition, force_decode
    4744from sphinx.util.docstrings import prepare_docstring
    48 from sphinx.util.inspect import Signature, isdescriptor, safe_getmembers, \
     45from sphinx.util.inspect import isdescriptor, safe_getmembers, \
    4946    safe_getattr, object_description, is_builtin_class_method, \
    5047    isenumattribute, isclassmethod, isstaticmethod, getdoc
    51 from sphinx.util.inspect import getargspec
    5248
    5349from sage.misc.sageinspect import (sage_getdoc_original,
    5450                                   sage_getargspec, isclassinstance,

or at least, it fixes the ones that ought to be fixed.

Last edited 6 months ago by jhpalmieri (previous) (diff)

comment:42 in reply to: ↑ 40 Changed 6 months ago by embray

Replying to jhpalmieri:

After I fix the references in dyck_word.py, I get a ton of other "Duplicate citation" and "Duplicate explicit target names" from other files in sage/combinat. For example:

I didn't have this problem before, so is it possible it was recently introduced? Perhaps it's just another citation that needs to be moved to the module-level, or the master citations list?

comment:43 Changed 6 months ago by jhpalmieri

It may only appear if you do make doc-clean. My guess is that whatever is forcing us to move the references in dyck_word.py to the module-level is also going to force us to move other references. The references in FullyPackedLoop are in a class with the decoration @add_metaclass(InheritComparisonClasscallMetaclass). The reference Combe2019 is in a method with @cached_method. Do we have to move all references in classes, methods, with these decorations to the module-level?

comment:44 Changed 6 months ago by jhpalmieri

Another issue: in tableau_tuple.py we have

from sage.combinat.tableau import (Tableau, ...)

and later

class TableauTuple(CombinatorialElement):
    ...
    Element = Tableau

and now Sphinx thinks that all of the references in the Element class are duplicates of those from Tableau. This seems to be fixed by moving all of the references to the master bibliography file.

comment:45 Changed 6 months ago by jhpalmieri

  • Branch changed from u/embray/python3/ticket-27692 to u/jhpalmieri/python3/ticket-27692

comment:46 Changed 6 months ago by jhpalmieri

  • Commit changed from 8337e67599e7e3e96c705fd48fd102123ace9dc4 to 4475f3072623e8a5f7a8e35d3fdcb4116e7990ca

I'm getting some doctest failures with Python 3. Does anyone else see them?

----------------------------------------------------------------------
sage -t --long src/sage/categories/category.py  # 5 doctests failed
sage -t --long src/sage/categories/primer.py  # 1 doctest failed
sage -t --long src/sage/categories/category_with_axiom.py  # 5 doctests failed
sage -t --long src/sage/categories/homsets.py  # 1 doctest failed
sage -t --long src/sage/categories/polyhedra.py  # 1 doctest failed
----------------------------------------------------------------------

For example:

sage -t --long src/sage/categories/homsets.py
**********************************************************************
File "src/sage/categories/homsets.py", line 56, in sage.categories.homsets.HomsetsCategory.default_super_categories
Failed example:
    type(H)
Expected:
    <class 'sage.categories.additive_monoids.AdditiveMonoids.Homsets_with_category'>
Got:
    <class 'sage.categories.additive_monoids.Homsets_with_category'>

New commits:

6ce1df6trac 27692: pyflakes cleanup in sage_autodoc.py
4475f30trac 27692: move some references to master bibliography file
Last edited 6 months ago by jhpalmieri (previous) (diff)

comment:47 Changed 6 months ago by chapoton

I can confirm the same failure in categories/ with the present branch here.

comment:48 Changed 6 months ago by chapoton

  • Status changed from needs_review to needs_work

This has doctests failure with python3

comment:49 Changed 5 months ago by embray

  • Milestone changed from sage-8.8 to sage-8.9

Moving tickets from the Sage 8.8 milestone that have been actively worked on in the last six months to the next release milestone (optimistically).

comment:50 Changed 5 months ago by chapoton

Is there any hope for progress here ?

EDIT: the branch is now red

Last edited 5 months ago by chapoton (previous) (diff)

comment:51 Changed 5 months ago by chapoton

I have made #28105 for just moving the references.

comment:52 Changed 5 months ago by chapoton

Erik, John ? The branch is now red. Is there any hope to make at least a partial progress on this file ?

comment:53 in reply to: ↑ 28 Changed 5 months ago by jhpalmieri

Replying to embray:

There does appear to be a legitimate issue here w.r.t. Sphinx.

In the class QuasiSymmetricFunctions the nested class Fundamental also has an alias of just F. That is, in the class body it has:

F = Fundamental

just as a shortcut. This does not affect the qualified name of the class, which is still correct:

sage: QuasiSymmetricFunctions.Fundamental.__qualname__
'QuasiSymmetricFunctions.Fundamental'

but for some reason Sphinx is outputting the docs for QuasiSymmetricFunctions with this nested class listed as just "F" instead of "Fundamental", whereas in the current docs it's listed correctly as "Fundamental".

I don't see this problem, at least with a Python 3 build. I see

F
    alias of QuasiSymmetricFunctions.Fundamental

class Fundamental(QSym)
    ...

Without this branch, the first two lines are missing, but everything else looks the same.

comment:54 Changed 5 months ago by jhpalmieri

On the other hand, with this branch I see a new py3 doctest failure:

File "src/sage/misc/c3_controlled.pyx", line 449, in sage.misc.c3_controlled.CmpKey
Failed example:
    Sets().Infinite()._cmp_key
Expected:
    (4, ...)
Got:
    (0, 40)
**********************************************************************
1 item had failures:
   1 of  27 in sage.misc.c3_controlled.CmpKey

comment:55 Changed 5 months ago by git

  • Commit changed from 4475f3072623e8a5f7a8e35d3fdcb4116e7990ca to e8a9b9fafceec5d00661410386e247801441339e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

67f6132py3: Make NestedClassMetaclass do nothing on Python 3
5f63e7dFix up sage_autodoc to work with normal Python 3 nested classes with qualnames,
e8a9b9ftrac 27692: pyflakes cleanup in sage_autodoc.py

comment:56 Changed 5 months ago by jhpalmieri

Rebased to 8.9.beta2.

comment:57 Changed 5 months ago by jhpalmieri

In my opinion, if the new doctest failure in c3_controlled.pyx can be fixed, then this is ready to go. I don't know what's causing it, though.

comment:58 Changed 5 months ago by chapoton

This seems to be an issue with class name of Sets().Infinite()

in the flags tables, one sees

{'FacadeSets': 1,
 'FiniteSets': 2,
 'Sets.Infinite': 4,
 'EnumeratedSets': 8,

where the second line is the only line with a dot inside.

comment:59 Changed 5 months ago by jhpalmieri

It may be more complicated than that. I made this change:

  • src/sage/misc/c3_controlled.pyx

    diff --git a/src/sage/misc/c3_controlled.pyx b/src/sage/misc/c3_controlled.pyx
    index 323086fb8e..9b404e8b70 100644
    a b from sage.structure.dynamic_class import dynamic_class 
    375375##############################################################################
    376376
    377377cdef tuple atoms = ("FacadeSets",
    378                     "FiniteSets", "Sets.Infinite", "EnumeratedSets", "SetsWithGrading",
     378                    "FiniteSets", "InfiniteSets", "EnumeratedSets", "SetsWithGrading",
    379379                    "Posets", "LatticePosets", "Crystals", "AdditiveMagmas",
    380380                    "FiniteDimensionalModules", "GradedModules", "ModulesWithBasis",
    381381                    "Magmas", "Semigroups", "Monoids", "PermutationGroups",

and then added a statement to print flags. I got this:

{'FacadeSets': 1, 'FiniteSets': 2, 'InfiniteSets': 4, 'EnumeratedSets': 8, 'SetsWithGrading': 16, 'Posets': 32, 'LatticePosets': 64, 'Crystals': 128, 'AdditiveMagmas': 256, 'FiniteDimensionalModules': 512, 'GradedModules': 1024, 'ModulesWithBasis': 2048, 'Magmas': 4096, 'Semigroups': 8192, 'Monoids': 16384, 'PermutationGroups': 32768, 'MagmasAndAdditiveMagmas': 65536, 'Rngs': 131072, 'Domains': 262144, 'HopfAlgebras': 524288}
sage: Sets().Finite()._cmp_key
(2, 55)
sage: Sets().Infinite()._cmp_key
(0, 40)
sage: Sets().Enumerated()._cmp_key
(8, 42)

So although InfiniteSets is now in flags with value of 4, the _cmp_key is still 0 for Sets().Infinite().

comment:60 Changed 5 months ago by jhpalmieri

Without this branch:

sage: C = Sets().Infinite()
sage: C.__class__.__name__
'Sets.Infinite_with_category'

With this branch:

sage: C = Sets().Infinite()
sage: C.__class__.__name__
'Infinite_with_category'

Changing Sets.Infinite to just Infinite in atoms fixes the doctest, but it doesn't feel like the right thing to do.

Last edited 5 months ago by jhpalmieri (previous) (diff)

comment:61 Changed 5 months ago by jhpalmieri

This seems like a direct consequence of the change in nested_class.pyx. Do we do:

if PY2:
    ... define atoms using Sets.Infinite ...
else:
    ... define atoms using Infinite ...

Seems ugly.

comment:62 Changed 5 months ago by jhpalmieri

For example:

  • src/sage/misc/c3_controlled.pyx

    diff --git a/src/sage/misc/c3_controlled.pyx b/src/sage/misc/c3_controlled.pyx
    index 323086fb8e..2c7cd0f573 100644
    a b from sage.misc.classcall_metaclass import ClasscallMetaclass, typecall 
    369369from sage.misc.cachefunc import cached_function, cached_method
    370370from sage.misc.lazy_attribute import lazy_attribute
    371371from sage.structure.dynamic_class import dynamic_class
     372from six import PY2
    372373
    373374##############################################################################
    374375# Implementation of the total order between categories
    375376##############################################################################
    376377
     378if PY2:
     379    infinitesets = "Sets.Infinite"
     380else:
     381    infinitesets = "Infinite"
     382
    377383cdef tuple atoms = ("FacadeSets",
    378                     "FiniteSets", "Sets.Infinite", "EnumeratedSets", "SetsWithGrading",
     384                    "FiniteSets", infinitesets, "EnumeratedSets", "SetsWithGrading",
    379385                    "Posets", "LatticePosets", "Crystals", "AdditiveMagmas",
    380386                    "FiniteDimensionalModules", "GradedModules", "ModulesWithBasis",
    381387                    "Magmas", "Semigroups", "Monoids", "PermutationGroups",

I don't like it, but it works.

comment:63 Changed 5 months ago by klee

  • Branch changed from u/jhpalmieri/python3/ticket-27692 to public/27692
  • Commit changed from e8a9b9fafceec5d00661410386e247801441339e to fb6d5fa684eb8ba8ddce9c38211cfe1e2e83d2e8

New commits:

4214075py3: Make NestedClassMetaclass do nothing on Python 3
291d584Move this reference to the module level.
8337e67Fix up sage_autodoc to work with normal Python 3 nested classes with qualnames,
6ce1df6trac 27692: pyflakes cleanup in sage_autodoc.py
4475f30trac 27692: move some references to master bibliography file
78ab9c5Merge branch 'develop'
fb6d5faKeep NestedClassMetaClass functional even with Python 3

comment:64 Changed 5 months ago by klee

In the commit fb6d5fa, I followed different approach to solve the issue. I keep NestedClassMetaclass as it was even with python3, but fixes doctests appropriately for both python2 and python3. See the note in the module docstring.

I think this approach is simple and the right one at this stage.

To a reviewer: I am not completely sure about what I wrote in the note. Please correct the note.

comment:65 Changed 5 months ago by klee

  • Status changed from needs_work to needs_review

comment:66 Changed 5 months ago by jhpalmieri

Tests pass for me. The approach seems okay to me, but I am not an expert on this aspect of the code. What do others think? @embray?

comment:67 Changed 5 months ago by klee

I add that Erik's approach should be considered when we drop the support on python2 and remove the nested class module altogether.

comment:68 follow-up: Changed 5 months ago by jhpalmieri

It would be nice to improve the documentation. This is not a new addition, but what do "save_global" and "load_global" refer to? I guess they are methods in Python's pickle module, but they are undocumented, which makes them less than ideal as reference points. (Fixing this could certainly wait until another ticket.)

comment:69 Changed 5 months ago by jhpalmieri

The only issue here is the note added by klee. Is it correct? If so, I think this is ready to go.

comment:70 Changed 5 months ago by git

  • Commit changed from fb6d5fa684eb8ba8ddce9c38211cfe1e2e83d2e8 to 04645f15a23259ec14ae054ae846bc6c29245a45

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

04645f1Correct the note

comment:71 in reply to: ↑ 68 ; follow-up: Changed 5 months ago by klee

Replying to jhpalmieri:

It would be nice to improve the documentation. This is not a new addition, but what do "save_global" and "load_global" refer to? I guess they are methods in Python's pickle module, but they are undocumented, which makes them less than ideal as reference points. (Fixing this could certainly wait until another ticket.)

They are C functions in cPickle module (actually cPickle.c), and not visible to a user.

comment:72 Changed 5 months ago by klee

In the last commit, I corrected the note, after investigating cPickle of python2 and the pickle module of python3. So I am now sure that what it says is correct.

I think this ticket is now good to go.

comment:73 in reply to: ↑ 71 ; follow-up: Changed 5 months ago by jhpalmieri

Replying to klee:

Replying to jhpalmieri:

It would be nice to improve the documentation. This is not a new addition, but what do "save_global" and "load_global" refer to? I guess they are methods in Python's pickle module, but they are undocumented, which makes them less than ideal as reference points. (Fixing this could certainly wait until another ticket.)

They are C functions in cPickle module (actually cPickle.c), and not visible to a user.

Exactly. Not only not visible to a user, but as I said, not documented anywhere, so having them as part of our documentation is not helpful.

comment:74 Changed 5 months ago by git

  • Commit changed from 04645f15a23259ec14ae054ae846bc6c29245a45 to 040265b3d75faac9df1b5618ed1035943edb1de9

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

040265bRemove mentioning undocumented functions in cPickle

comment:75 in reply to: ↑ 73 Changed 5 months ago by klee

Replying to jhpalmieri:

Replying to klee:

Replying to jhpalmieri:

It would be nice to improve the documentation. This is not a new addition, but what do "save_global" and "load_global" refer to? I guess they are methods in Python's pickle module, but they are undocumented, which makes them less than ideal as reference points. (Fixing this could certainly wait until another ticket.)

They are C functions in cPickle module (actually cPickle.c), and not visible to a user.

Exactly. Not only not visible to a user, but as I said, not documented anywhere, so having them as part of our documentation is not helpful.

Ok. Then let's remove them here.

comment:76 Changed 5 months ago by chapoton

  • Authors changed from Erik Bray to Erik Bray, Kwankyu Lee
  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, John Palmieri, Kwankyu Lee
  • Status changed from needs_review to positive_review

I think that it is more than time to move on here.

Given that John and Kwankyu said "good to go", I am setting to positive

comment:77 Changed 5 months ago by vbraun

  • Status changed from positive_review to needs_work

Now it fails on py2 with:

[dochtml] OSError: /var/lib/buildbot/slave/sage_git/build/local/lib/python2.7/site-packages/sage/combinat/tableau.py:docstring of sage.combinat.tableau.Tableaux.Element.promotion:40: WARNING: Duplicate explicit target name: "hai1992".

comment:78 Changed 5 months ago by git

  • Commit changed from 040265b3d75faac9df1b5618ed1035943edb1de9 to 86df126957ba2b27bd97dcf90887f5e4608476cf

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

a1e3a4cMerge branch 'develop' into t/27692/public/27692
86df126trac 27692: fix one citation

comment:79 Changed 5 months ago by jhpalmieri

  • Status changed from needs_work to positive_review

This should fix the problem.

comment:80 Changed 4 months ago by vbraun

  • Status changed from positive_review to needs_work

Now the pdf docs are broken

[docpdf] Underfull \hbox (badness 10000) in paragraph at lines 4766--4767
[docpdf] \T1/ptm/m/n/10 Bases: [][]\T1/pcr/m/n/10 sage.categories.category_with_axiom.
[docpdf] 
[docpdf] ! LaTeX Error: Too deeply nested.
[docpdf] 
[docpdf] See the LaTeX manual or LaTeX Companion for explanation.
[docpdf] Type  H <return>  for immediate help.
[docpdf]  ...                                              
[docpdf]                                                   
[docpdf] l.4779 \begin{itemize}
[docpdf]                       
[docpdf] ? 
[docpdf] ! Emergency stop.
[docpdf]  ...                                              

comment:81 Changed 4 months ago by git

  • Commit changed from 86df126957ba2b27bd97dcf90887f5e4608476cf to 2f3fc35b3806fdd731bf5b90f11ff95df389eae8

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

13eb46fMerge branch 'develop'
fb30abbMerge branch 'public/27692'
fefe75eFix class documenter in sage_autodoc
2f3fc35Recover some original docstrings

comment:82 Changed 4 months ago by klee

Seems to have fixed pdf docs.

comment:83 Changed 4 months ago by klee

  • Status changed from needs_work to needs_review

comment:84 Changed 4 months ago by git

  • Commit changed from 2f3fc35b3806fdd731bf5b90f11ff95df389eae8 to d8cc2149816f2045f8476bd61acf4550d8c6698a

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

d8cc214Make aliases documented

comment:85 Changed 4 months ago by klee

The last commit makes aliases of nested classes documented.

It looks all good to me. But the nested class stuff of Sage is very subtle, and these changes might have unexpected effects at unexpected places in the documentation.

Please check the generated docs carefully, before we move on.

comment:86 Changed 4 months ago by fbissey

Needs rebasing on top of 8.9.beta4.

comment:87 Changed 4 months ago by git

  • Commit changed from d8cc2149816f2045f8476bd61acf4550d8c6698a to 562ff1e1f69908bdc5a622a61510d5bdea8c51b2

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

562ff1eFix nested_class for python 3

comment:88 Changed 4 months ago by klee

  • Reviewers changed from Frédéric Chapoton, John Palmieri, Kwankyu Lee to Frédéric Chapoton, John Palmieri

comment:89 Changed 4 months ago by jhpalmieri

With the new version, attributes appear in the reference manual. This is a change, but it is fine with me.

comment:90 Changed 4 months ago by jhpalmieri

  • Status changed from needs_review to positive_review

That's the only real difference, as far as I can tell. There may be some small differences in the indices (files like http://doc.sagemath.org/html/en/reference/genindex-A.html), as well, but very minor ones. I am happy with this.

(Edit: I built docs with both Python 2 and Python 3, with and without this branch, and ran diff -r ... on the resulting doc directories, so I could see all of the differences. I only checked the html and latex output.)

Last edited 4 months ago by jhpalmieri (previous) (diff)

comment:91 Changed 4 months ago by klee

I am also happy with more documentation. Just for the record, more members of a class, like attributes and aliases, now appear in the documentation because of a change in skip_member function defined in sage/docs/conf.py, which used to skip them for documentation:

     objname = getattr(obj, "__name__", None)
     if objname is not None:
-        if objname.find('.') != -1 and objname.split('.')[-1] != name:
-            return True
+        # check if name was inserted to the module by NestedClassMetaclass
+        if name.find('.') != -1 and objname.find('.') != -1:
+            if objname.split('.')[-1] == name.split('.')[-1]:
+                return True
 
     if name.find("userchild_download_worksheets.zip") != -1:
         return True

comment:92 Changed 4 months ago by vbraun

  • Branch changed from public/27692 to 562ff1e1f69908bdc5a622a61510d5bdea8c51b2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.