Opened 3 years ago
Closed 3 years 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, GitHub, GitLab) | 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 3 years ago by
comment:2 Changed 3 years ago by
At some point I definitely fixed or changed something related to this but I'm wracking my brain to remember where.
comment:3 Changed 3 years ago by
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 3 years ago by
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:: 36 36 37 37 sage: A1.A2.A3.__name__ 38 38 'A3' 39 sage: A1.A2.A3 39 sage: A1.A2.A3 # py2 40 40 <class sage.misc.nested_class.A3 at ...> 41 41 42 sage: nested_pickle(A1) 42 sage: nested_pickle(A1) # py2 43 43 <class sage.misc.nested_class.A1 at ...> 44 44 45 sage: A1.A2 45 sage: A1.A2 # py2 46 46 <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'> 47 49 48 sage: A1.A2.A3 50 sage: A1.A2.A3 # py2 49 51 <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 51 55 'A1.A2.A3' 52 56 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 54 58 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 56 60 True 57 61 58 62 All of this is not perfect. In the following scenario:: … … cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True): 85 89 giving them a mangled name and putting the mangled name in the 86 90 module namespace. 87 91 92 On Python 3 this is a no-op and does not perform any mangling. 93 88 94 INPUT: 89 95 90 96 - ``cls`` - The class to modify. … … cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True): 111 117 sage: getattr(module, 'A.B', 'Not found') 112 118 'Not found' 113 119 sage: modify_for_nested_pickle(A, 'A', sys.modules['__main__']) 114 sage: A.B.__name__ 120 sage: A.B.__name__ # py2 115 121 'A.B' 116 sage: getattr(module, 'A.B', 'Not found') 122 sage: getattr(module, 'A.B', 'Not found') # py2 117 123 <class '__main__.A.B'> 118 124 119 125 Here we demonstrate the effect of the ``first_run`` argument:: 120 126 121 127 sage: modify_for_nested_pickle(A, 'X', sys.modules['__main__']) 122 sage: A.B.__name__ # nothing changed128 sage: A.B.__name__ # py2: nothing changed 123 129 'A.B' 124 130 sage: modify_for_nested_pickle(A, 'X', sys.modules['__main__'], first_run=False) 125 sage: A.B.__name__ 131 sage: A.B.__name__ # py2 126 132 'X.A.B' 127 133 128 134 Note that the class is now found in the module under both its old and 129 135 its new name:: 130 136 131 sage: getattr(module, 'A.B', 'Not found') 137 sage: getattr(module, 'A.B', 'Not found') # py2 132 138 <class '__main__.X.A.B'> 133 sage: getattr(module, 'X.A.B', 'Not found') 139 sage: getattr(module, 'X.A.B', 'Not found') # py2 134 140 <class '__main__.X.A.B'> 135 141 136 142 … … cpdef modify_for_nested_pickle(cls, str name_prefix, module, first_run=True): 151 157 152 158 Before :trac:`9107`, the name of ``A1.B1.C1`` would have been wrong:: 153 159 154 sage: A1.B1.C1.__name__ 160 sage: A1.B1.C1.__name__ # py2 155 161 'A1.B1.C1' 156 sage: A1.B2.C2.__name__ 162 sage: A1.B2.C2.__name__ # py2 157 163 'A1.B2.C2' 158 164 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 160 166 '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 162 168 'A1.B2.C2' 163 169 164 170 """ 171 IF PY_MAJOR_VERSION >= 3: 172 return 173 165 174 cdef str name, dotted_name 166 175 cdef str mod_name = module.__name__ 167 176 cdef str cls_name = cls.__name__+'.' … … def nested_pickle(cls): 214 223 then the name of class ``"B"`` will be modified to ``"A.B"``, and the ``"A.B"`` 215 224 attribute of the module will be set to class ``"B"``:: 216 225 217 sage: A.B.__name__ 226 sage: A.B.__name__ # py2 218 227 'A.B' 219 sage: getattr(module, 'A.B', 'Not found') 228 sage: getattr(module, 'A.B', 'Not found') # py2 220 229 <class __main__.A.B at ...> 221 230 222 231 In Python 2.6, decorators work with classes; then ``@nested_pickle`` … … cdef class NestedClassMetaclass(type): 245 254 r""" 246 255 A metaclass for nested pickling. 247 256 257 On Python 3 this is just a direct wrapper around the base `type` and does 258 not do anything. 259 248 260 Check that one can use a metaclass to ensure nested_pickle 249 261 is called on any derived subclass:: 250 262 … … cdef class NestedClassMetaclass(type): 256 268 ....: class B(object): 257 269 ....: pass 258 270 ... 259 sage: A3.B.__name__ 271 sage: A3.B.__name__ # py2 260 272 'A3.B' 261 sage: getattr(sys.modules['__main__'], 'A3.B', 'Not found') 273 sage: getattr(sys.modules['__main__'], 'A3.B', 'Not found') # py2 262 274 <class '__main__.A3.B'> 263 275 """ 276 264 277 def __init__(self, *args): 265 278 r""" 266 279 This invokes the nested_pickle on construction. … … cdef class NestedClassMetaclass(type): 273 286 ... 274 287 sage: A.B 275 288 <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 277 290 <class '__main__.A.B'> 278 291 """ 279 292 modify_for_nested_pickle(self, self.__name__, sys_modules[self.__module__]) … … class MainClass(object, metaclass=NestedClassMetaclass): 306 319 sage: from sage.misc.nested_class import * 307 320 sage: loads(dumps(MainClass.NestedClass.NestedSubClass())) 308 321 <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 310 323 <class 'sage.misc.nested_class.MainClass.NestedClass.NestedSubClass'> 311 sage: MainClass.NestedClass.NestedSubClass.__name__ 324 sage: MainClass.NestedClass.NestedSubClass.__name__ # py2 312 325 'MainClass.NestedClass.NestedSubClass' 313 326 """ 314 327 def dummy(self, x, *args, r=(1,2,3.4), **kwds): … … class MainClass(object, metaclass=NestedClassMetaclass): 321 334 sage: from sage.misc.nested_class import MainClass 322 335 sage: print(MainClass.NestedClass.NestedSubClass.dummy.__doc__) 323 336 NestedSubClass.dummy(self, x, *args, r=(1, 2, 3.4), **kwds) 324 File: sage/misc/nested_class.pyx (starting at line 3 14)337 File: sage/misc/nested_class.pyx (starting at line 327) 325 338 <BLANKLINE> 326 339 A dummy method to demonstrate the embedding of 327 340 method signature for nested classes.
comment:5 Changed 3 years ago by
And most of the tests are skipped on Python 3 as they are not relevant.
comment:6 Changed 3 years ago by
branch please
comment:7 Changed 3 years ago by
- 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:
c876038 | py3: Make NestedClassMetaclass do nothing on Python 3
|
comment:8 Changed 3 years ago by
one failing doctest in src/sage/misc/sageinspect.py, see patchbot
comment:9 Changed 3 years ago by
- Status changed from needs_review to needs_work
comment:10 Changed 3 years ago by
- 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
comment:11 Changed 3 years ago by
Right...there's always one of those...
comment:12 Changed 3 years ago by
- 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:
174e895 | py3: Make NestedClassMetaclass do nothing on Python 3
|
comment:14 Changed 3 years ago by
- Branch changed from u/embray/python3/ticket-27692 to 174e8956aa38a325ec189f786c4ee27c246c55fa
- Resolution set to fixed
- Status changed from positive_review to closed
comment:15 Changed 3 years ago by
- 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:
174e895 | py3: Make NestedClassMetaclass do nothing on Python 3
|
comment:16 Changed 3 years ago by
- Owner changed from (none) to embray
comment:17 Changed 3 years ago by
- Status changed from new to needs_info
comment:18 Changed 3 years ago by
- Status changed from needs_info to needs_work
Not sure why I can't just progress straight to needs_work...
comment:19 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
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: 62 62 Quasisymmetric Functions Expand Positively into Young Quasisymmetric 63 63 Schur Functions*. :arxiv:`1606.03519` 64 64 65 .. [SW2010] John Shareshian and Michelle Wachs. 66 *Eulerian quasisymmetric functions*. (2010). 67 :arxiv:`0812.0764v2` 68 65 69 AUTHOR: 66 70 67 71 - Jason Bandlow … … class QuasiSymmetricFunctions(UniqueRepresentation, Parent): 2348 2352 - ``k`` -- (optional) if specified, determines the number of fixed 2349 2353 points of the permutations which are being summed over 2350 2354 2351 REFERENCES:2352 2353 .. [SW2010] John Shareshian and Michelle Wachs.2354 *Eulerian quasisymmetric functions*. (2010).2355 :arxiv:`0812.0764v2`2356 2357 2355 EXAMPLES:: 2358 2356 2359 2357 sage: F = QuasiSymmetricFunctions(QQ).F()
Again, this is just a bandaid.
comment:22 Changed 3 years ago by
Good job on Volker to find it out. I couldn't figure which ticket was causing that breakage.
comment:23 Changed 3 years ago by
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 3 years ago by
Or, I guess maybe I'm thinking of #27497, which is not exactly as I described.
comment:25 Changed 3 years ago by
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: ↓ 27 Changed 3 years ago by
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 3 years ago by
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: ↓ 53 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
Is there any hope for progress here ? This is now the top-failing file, by number of failures.
comment:31 Changed 3 years ago by
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 3 years ago by
- Commit changed from 174e8956aa38a325ec189f786c4ee27c246c55fa to 42140750b92b7827d3619d0c3c6abbd9b2790292
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4214075 | py3: Make NestedClassMetaclass do nothing on Python 3
|
comment:33 Changed 3 years ago by
Rebased. Going to keep investigating the docbuild issue now.
comment:34 Changed 3 years ago by
- Commit changed from 42140750b92b7827d3619d0c3c6abbd9b2790292 to 291d58442794d76d904404d9b58245d98d53ed18
Branch pushed to git repo; I updated commit sha1. New commits:
291d584 | Move this reference to the module level.
|
comment:35 Changed 3 years ago by
- Commit changed from 291d58442794d76d904404d9b58245d98d53ed18 to 8337e67599e7e3e96c705fd48fd102123ace9dc4
Branch pushed to git repo; I updated commit sha1. New commits:
8337e67 | Fix up sage_autodoc to work with normal Python 3 nested classes with qualnames,
|
comment:36 Changed 3 years ago by
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 3 years ago by
- 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 3 years ago by
This looks okay to me, but others should investigate, too.
comment:39 Changed 3 years ago by
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: ↓ 42 Changed 3 years ago by
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 3 years ago by
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: 30 30 import inspect 31 31 import re 32 32 import sys 33 import warnings34 33 35 34 from docutils.statemachine import ViewList 36 from six import PY2, iter items, itervalues, text_type, class_types, string_types35 from six import PY2, itervalues, text_type, class_types, string_types 37 36 38 37 import sphinx 39 from sphinx.errors import ExtensionError40 38 from sphinx.ext.autodoc.importer import mock, import_object, get_object_members 41 from sphinx.ext.autodoc.inspector import format_annotation42 39 from sphinx.locale import _, __ 43 40 from sphinx.pycode import ModuleAnalyzer 44 from sphinx.errors import ExtensionError,PycodeError41 from sphinx.errors import PycodeError 45 42 from sphinx.util import logging 46 43 from sphinx.util import rpartition, force_decode 47 44 from sphinx.util.docstrings import prepare_docstring 48 from sphinx.util.inspect import Signature,isdescriptor, safe_getmembers, \45 from sphinx.util.inspect import isdescriptor, safe_getmembers, \ 49 46 safe_getattr, object_description, is_builtin_class_method, \ 50 47 isenumattribute, isclassmethod, isstaticmethod, getdoc 51 from sphinx.util.inspect import getargspec52 48 53 49 from sage.misc.sageinspect import (sage_getdoc_original, 54 50 sage_getargspec, isclassinstance,
or at least, it fixes the ones that ought to be fixed.
comment:42 in reply to: ↑ 40 Changed 3 years ago by
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 insage/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 3 years ago by
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 3 years ago by
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 3 years ago by
- Branch changed from u/embray/python3/ticket-27692 to u/jhpalmieri/python3/ticket-27692
comment:46 Changed 3 years ago by
- 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:
6ce1df6 | trac 27692: pyflakes cleanup in sage_autodoc.py
|
4475f30 | trac 27692: move some references to master bibliography file
|
comment:47 Changed 3 years ago by
I can confirm the same failure in categories/ with the present branch here.
comment:48 Changed 3 years ago by
- Status changed from needs_review to needs_work
This has doctests failure with python3
comment:49 Changed 3 years ago by
- 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 3 years ago by
Is there any hope for progress here ?
EDIT: the branch is now red
comment:51 Changed 3 years ago by
I have made #28105 for just moving the references.
comment:52 Changed 3 years ago by
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 3 years ago by
Replying to embray:
There does appear to be a legitimate issue here w.r.t. Sphinx.
In the class
QuasiSymmetricFunctions
the nested classFundamental
also has an alias of justF
. That is, in the class body it has:F = Fundamentaljust 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 3 years ago by
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 3 years ago by
- Commit changed from 4475f3072623e8a5f7a8e35d3fdcb4116e7990ca to e8a9b9fafceec5d00661410386e247801441339e
comment:56 Changed 3 years ago by
Rebased to 8.9.beta2.
comment:57 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
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 375 375 ############################################################################## 376 376 377 377 cdef tuple atoms = ("FacadeSets", 378 "FiniteSets", " Sets.Infinite", "EnumeratedSets", "SetsWithGrading",378 "FiniteSets", "InfiniteSets", "EnumeratedSets", "SetsWithGrading", 379 379 "Posets", "LatticePosets", "Crystals", "AdditiveMagmas", 380 380 "FiniteDimensionalModules", "GradedModules", "ModulesWithBasis", 381 381 "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 3 years ago by
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.
comment:61 Changed 3 years ago by
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 3 years ago by
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 369 369 from sage.misc.cachefunc import cached_function, cached_method 370 370 from sage.misc.lazy_attribute import lazy_attribute 371 371 from sage.structure.dynamic_class import dynamic_class 372 from six import PY2 372 373 373 374 ############################################################################## 374 375 # Implementation of the total order between categories 375 376 ############################################################################## 376 377 378 if PY2: 379 infinitesets = "Sets.Infinite" 380 else: 381 infinitesets = "Infinite" 382 377 383 cdef tuple atoms = ("FacadeSets", 378 "FiniteSets", "Sets.Infinite", "EnumeratedSets", "SetsWithGrading",384 "FiniteSets", infinitesets, "EnumeratedSets", "SetsWithGrading", 379 385 "Posets", "LatticePosets", "Crystals", "AdditiveMagmas", 380 386 "FiniteDimensionalModules", "GradedModules", "ModulesWithBasis", 381 387 "Magmas", "Semigroups", "Monoids", "PermutationGroups",
I don't like it, but it works.
comment:63 Changed 3 years ago by
- Branch changed from u/jhpalmieri/python3/ticket-27692 to public/27692
- Commit changed from e8a9b9fafceec5d00661410386e247801441339e to fb6d5fa684eb8ba8ddce9c38211cfe1e2e83d2e8
New commits:
4214075 | py3: Make NestedClassMetaclass do nothing on Python 3
|
291d584 | Move this reference to the module level.
|
8337e67 | Fix up sage_autodoc to work with normal Python 3 nested classes with qualnames,
|
6ce1df6 | trac 27692: pyflakes cleanup in sage_autodoc.py
|
4475f30 | trac 27692: move some references to master bibliography file
|
78ab9c5 | Merge branch 'develop'
|
fb6d5fa | Keep NestedClassMetaClass functional even with Python 3
|
comment:64 Changed 3 years ago by
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 3 years ago by
- Status changed from needs_work to needs_review
comment:66 Changed 3 years ago by
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 3 years ago by
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: ↓ 71 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
- Commit changed from fb6d5fa684eb8ba8ddce9c38211cfe1e2e83d2e8 to 04645f15a23259ec14ae054ae846bc6c29245a45
Branch pushed to git repo; I updated commit sha1. New commits:
04645f1 | Correct the note
|
comment:71 in reply to: ↑ 68 ; follow-up: ↓ 73 Changed 3 years ago by
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 3 years ago by
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: ↓ 75 Changed 3 years ago by
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 3 years ago by
- Commit changed from 04645f15a23259ec14ae054ae846bc6c29245a45 to 040265b3d75faac9df1b5618ed1035943edb1de9
Branch pushed to git repo; I updated commit sha1. New commits:
040265b | Remove mentioning undocumented functions in cPickle
|
comment:75 in reply to: ↑ 73 Changed 3 years ago by
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 3 years ago by
- 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 3 years ago by
- 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 3 years ago by
- Commit changed from 040265b3d75faac9df1b5618ed1035943edb1de9 to 86df126957ba2b27bd97dcf90887f5e4608476cf
comment:79 Changed 3 years ago by
- Status changed from needs_work to positive_review
This should fix the problem.
comment:80 Changed 3 years ago by
- 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 3 years ago by
- Commit changed from 86df126957ba2b27bd97dcf90887f5e4608476cf to 2f3fc35b3806fdd731bf5b90f11ff95df389eae8
comment:82 Changed 3 years ago by
Seems to have fixed pdf docs.
comment:83 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:84 Changed 3 years ago by
- Commit changed from 2f3fc35b3806fdd731bf5b90f11ff95df389eae8 to d8cc2149816f2045f8476bd61acf4550d8c6698a
Branch pushed to git repo; I updated commit sha1. New commits:
d8cc214 | Make aliases documented
|
comment:85 Changed 3 years ago by
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 3 years ago by
Needs rebasing on top of 8.9.beta4.
comment:87 Changed 3 years ago by
- Commit changed from d8cc2149816f2045f8476bd61acf4550d8c6698a to 562ff1e1f69908bdc5a622a61510d5bdea8c51b2
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
562ff1e | Fix nested_class for python 3
|
comment:88 Changed 3 years ago by
- Reviewers changed from Frédéric Chapoton, John Palmieri, Kwankyu Lee to Frédéric Chapoton, John Palmieri
comment:89 Changed 3 years ago by
With the new version, attributes appear in the reference manual. This is a change, but it is fine with me.
comment:90 Changed 3 years ago by
- 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.)
comment:91 Changed 3 years ago by
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 3 years ago by
- Branch changed from public/27692 to 562ff1e1f69908bdc5a622a61510d5bdea8c51b2
- Resolution set to fixed
- Status changed from positive_review to closed
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.