Opened 11 years ago
Last modified 6 years ago
#9107 closed defect
Nested class name mangling can be wrong in case of double nesting — at Version 22
Reported by: | hivert | Owned by: | nthiery |
---|---|---|---|
Priority: | major | Milestone: | sage-6.3 |
Component: | categories | Keywords: | |
Cc: | SimonKing, zabrocki | Merged in: | |
Authors: | Simon King | Reviewers: | Volker Braun, Florent Hivert |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #12808 | Stopgaps: |
Description (last modified by )
In the following class tree:
class Bla(UniqueRepresentation): class Bla1(UniqueRepresentation): class Bla11: Pass class Bla2: class Bla21: Pass
The names are set to
sage: Bla.Bla1.__name__ 'Bla.Bla1' sage: Bla.Bla2.__name__ 'Bla.Bla2' sage: Bla.Bla2.Bla21.__name__ 'Bla.Bla2.Bla21'
But
sage: Bla.Bla1.Bla11.__name__ 'Bla1.Bla11'
whereas one would expect 'Bla.Bla1.Bla11'
This breaks a lot of doc in categories and in particular in functorial constructions.
Apply
Change History (24)
comment:1 Changed 10 years ago by
- Cc SimonKing added
comment:2 Changed 9 years ago by
- Dependencies set to #12808
comment:3 Changed 9 years ago by
- Status changed from new to needs_review
I think the attached patch solves the problem. I get:
sage: class Bla(UniqueRepresentation): ....: class Bla1(UniqueRepresentation): ....: class Bla11: ....: pass ....: class Bla2: ....: class Bla21: ....: pass ....: sage: Bla.Bla1.Bla11 <class __main__.Bla.Bla1.Bla11 at 0x46e7808>
The change is in modify_for_nested_pickle
, which is called recursively. The idea is that the function should have an extra argument first_run
, that is True by default. If the extra argument is False, then it is assumed that it is not applied for the first time.
Here: Since Bla.Bla1 is an instance of NestedClassMetaclass
, modify_for_nested_pickle
is called on Bla.Bla1.Bla11
, resulting in Bla.Bla1.Bla11.__name__=='Bla1.Bla11'
. However, since Bla is an instance of NestedClassMetaclass
as well, the function is applied to Bla.Bla1
and thus recursively to Bla.Bla1.Bla11
another time.
Now, without my patch, in the second run, modify_for_nested_pickle
would be confused by the fact that Bla.Bla1.__dict__
lists Bla.Bla1.Bla11
under the name Bla11
, but Bla11.__name__=='Bla1.Bla11'
. With my patch, modify_for_nested_pickle
expects exactly that naming scheme, and is thus changing Bla.Bla1.Bla11.__name__
into "Bla.Bla1.Bla11"
.
Much BlaBla, but I think it works...
Potential problems
sage: module = sys.modules['__main__'] sage: getattr(module, 'Bla1.Bla11') <class __main__.Bla.Bla1.Bla11 at 0x46e7808> sage: getattr(module, 'Bla.Bla1.Bla11') <class __main__.Bla.Bla1.Bla11 at 0x46e7808>
Hence, Bla.Bla1.Bla11 is listed in the module under two different names. If you think it is bad, then one could probably modify the function when first_run is false, such that the name given in the first run is erased from the module.
Moreover, the reviewer will likely find a speed regression, when excessively creating nested unique representations. But that's hardly realistic...
comment:4 Changed 9 years ago by
Another problem: Source inspection does not work yet in the following example.
sage: cython_code = [ ... "from sage.structure.unique_representation import UniqueRepresentation", ... "class A1(UniqueRepresentation):", ... " class B1(UniqueRepresentation):", ... " class C1: pass", ... " class B2:", ... " class C2: pass"] sage: import os sage: cython(os.linesep.join(cython_code)) sage: A1.B1.C1?? Error getting source: class A1.B1.C1 has no attribute '__class__' Type: classobj String Form: _mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx_0.A1.B1.C1 Namespace: Interactive Loaded File: /mnt/local/king/.sage/temp/mpc622/6475/spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx_0.so Source File: /mnt/local/king/.sage/temp/mpc622/6475/spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx/_mnt_local_king__sage_temp_mpc622_6475_tmp_0_spyx_0.so
Even #11768 does not solve the problem.
Shall that be dealt with on a different ticket? Or in one go?
Probably on a different ticket, since I just find that even source inspection for A1 (which has a usual name) does not work...
comment:5 Changed 9 years ago by
For the record: If #11791 is applied after this ticket, source inspection in the example above works (and is doctested).
comment:6 Changed 9 years ago by
Is there a reviewer to fix name mangling of nested classes (needed in the category framework)?
comment:7 Changed 9 years ago by
This patch also fixes an issue that came up in #8899 regarding documentation of nested classes not appearing in the reference manual.
See here for a description of the issue, see the thread on sage-combinat-devel.
See here for the confirmation that this works: http://trac.sagemath.org/sage_trac/ticket/8899#comment:31
comment:8 Changed 8 years ago by
- Reviewers set to Volker Braun
- Status changed from needs_review to positive_review
LGTM!
comment:9 Changed 8 years ago by
This causes trouble when building the documentation from scratch (i.e. after deleting 'devel/sage/doc/output`):
/usr/local/src/sage-5.5.rc1/local/lib/python2.7/site-packages/sage/categories/algebras_with_basis.py:docstring of sage.categories.algebras_with_basis.AlgebrasWithBasis.CartesianProducts.ParentMethods.one_from_cartesian_product_of_one_basis:3: WARNING: more than one target found for cross-reference u'one_basis': sage.combinat.sf.new_kschur.KBoundedSubspaceBases.ParentMethods.one_basis, sage.categories.algebras_with_basis.AlgebrasWithBasis.ParentMethods.one_basis, sage.combinat.ncsf_qsym.generic_basis_code.BasesOfQSymOrNCSF.ParentMethods.one_basis, sage.algebras.steenrod.steenrod_algebra.SteenrodAlgebra_generic.one_basis, sage.categories.examples.with_realizations.SubsetAlgebra.Fundamental.one_basis, sage.combinat.root_system.weyl_characters.WeightRing.one_basis, sage.categories.monoids.Monoids.Algebras.ParentMethods.one_basis, sage.categories.examples.hopf_algebras_with_basis.MyGroupAlgebra.one_basis, sage.categories.algebras_with_basis.AlgebrasWithBasis.TensorProducts.ParentMethods.one_basis, sage.algebras.affine_nil_temperley_lieb.AffineNilTemperleyLiebTypeA.one_basis, sage.categories.examples.algebras_with_basis.FreeAlgebra.one_basis, sage.combinat.symmetric_group_algebra.SymmetricGroupAlgebra_n.one_basis, sage.algebras.iwahori_hecke_algebra.IwahoriHeckeAlgebraT.one_basis, sage.algebras.group_algebra_new.GroupAlgebra.one_basis, sage.combinat.sf.sfa.SymmetricFunctionsBases.ParentMethods.one_basis, sage.combinat.root_system.weyl_characters.WeylCharacterRing.one_basis, sage.combinat.combinatorial_algebra.CombinatorialAlgebra.one_basis
comment:10 Changed 8 years ago by
- Status changed from positive_review to needs_work
comment:11 Changed 8 years ago by
Jeroen, can you elaborate a bit what went wrong?
comment:12 Changed 8 years ago by
Aha, now I see that the very long single line contains warnings about cross references that were not found. I'll try to identify them.
comment:13 Changed 8 years ago by
Aha, here is an example:
The docstring of sage.categories.algebras_with_basis.AlgebrasWithBasis.CartesianProducts.ParentMethods.one_from_cartesian_product_of_one_basis
is as follows:
@cached_method # todo: reinstate once #5843 is fixed def one_from_cartesian_product_of_one_basis(self): """ Returns the one of this cartesian product of algebras, as per ``Monoids.ParentMethods.one`` It is constructed as the cartesian product of the ones of the summands, using their :meth:`.one_basis` methods. This implementation does not require multiplication by scalars nor calling cartesian_product. This might help keeping things as lazy as possible upon initialization. ...
Could this simply be a misspelling? Note that it is written
:meth:`.one_basis`
but should certainly be
:meth:`one_basis`
If that's the case for the other warnings as well, then my patch would just uncover mistakes that happened earlier.
comment:14 follow-up: ↓ 15 Changed 8 years ago by
The same issue arose in #13851 (see comment 10). I'm not sure why those dots are there, and I personally think they should be removed, but someone intentionally put them there.
comment:15 in reply to: ↑ 14 Changed 8 years ago by
Replying to jhpalmieri:
The same issue arose in #13851 (see comment 10). I'm not sure why those dots are there, and I personally think they should be removed, but someone intentionally put them there.
I think the dot is simply wrong - or is it ignored by Sphinx?
Actually here it is even worse. The text is documentation of a functorial construction, but refers to a parent method - that can't possibly work without an explicit reference to the method which must include the class which the method belongs to.
comment:16 Changed 8 years ago by
- Status changed from needs_work to needs_review
Does the second patch fix the problem? I am now explicitly referring to the one_basis
method of AlgebrasWithBasis.ParentMethods
.
comment:17 Changed 8 years ago by
- Reviewers changed from Volker Braun to Volker Braun, Florent Hivert
- Status changed from needs_review to positive_review
Hi Simon,
I again hit this one compiling the doc. Your patches look all good to me, including the one problem.
Thanks,
Florent
comment:18 Changed 8 years ago by
- Status changed from positive_review to needs_work
Applying this patch causes the PDF docbuilder to hang after it's done building all documents. All documents are built but there are still 6 (I'm building with MAKE="make -j6"
) child processes which are stuck in the multiprocessing.Pool
code. Interrupting those child processes simply causes new child processes to start which are then stuck again. It might be a bug in multiprocessing.Pool
which is somehow triggered, I have no idea...
comment:19 Changed 8 years ago by
Perhaps an instance of #14323 (wild guess)?
comment:20 Changed 8 years ago by
No, #14323 doesn't help.
comment:21 Changed 8 years ago by
Jeroen, does the problem persist with the new doc builder? I have just applied the two patches, and succeeded with export MAKE="make -j2"
followed by make
.
However, there is continuation by ...
that needs fixing.
Changed 8 years ago by
comment:22 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Building the docs works for me, and the ...
should be fixed now. Hence: Needs review!
Apply trac9107_nesting_nested_classes.patch trac_9107_fix_cross_reference.patch
I think we should make this depend on #12808, because it cythonises nested classes.
Here is my analysis:
ClassType
. However, ironically, instances ofNestedClassMetaclass
are ignored.