Opened 8 years ago
Closed 7 years ago
#14793 closed enhancement (fixed)
Unique representation for homsets
Reported by:  nthiery  Owned by:  nthiery 

Priority:  major  Milestone:  sage6.2 
Component:  categories  Keywords:  
Cc:  sagecombinat, SimonKing, jpflori  Merged in:  
Authors:  Simon King  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  e2d2f16 (Commits, GitHub, GitLab)  Commit:  e2d2f167429b625b12690f91d1d8d69aaf69f91b 
Dependencies:  Stopgaps: 
Description (last modified by )
The unique representation of homsets is taken care of by Hom
. What's missing is:
 Fast hash and comparison by id. This can be implemented by having Homset inherit from WithEqualityById?
 Pickling by construction, calling back Hom(domain, codomain, category)
Attachments (2)
Change History (59)
comment:1 Changed 8 years ago by
 Cc SimonKing added
comment:2 Changed 8 years ago by
Observation: If one uses WithEqualityById
, many tests in sage.modular and sage.schemes fail with unprintable assertion errors.
We need to discuss whether we "simply" fix them, or better continue to compare homsets by *equality* (not identity) of domain and codomain.
In other words, we might end up with just providing a default pickling for homsets, which is currently missing.
comment:3 Changed 8 years ago by
When using WithEqualityByid
and a default __reduce__
method, we'd get (sorted by "section"):
Schemes:
sage t sage/schemes/elliptic_curves/padic_lseries.py # 1 doctest failed sage t sage/schemes/elliptic_curves/ell_curve_isogeny.py # 18 doctests failed sage t sage/schemes/elliptic_curves/ell_point.py # 8 doctests failed sage t sage/schemes/elliptic_curves/ell_finite_field.py # 1 doctest failed sage t sage/schemes/projective/projective_space.py # 2 doctests failed sage t sage/schemes/elliptic_curves/isogeny_class.py # 1 doctest failed sage t sage/schemes/affine/affine_space.py # 1 doctest failed sage t sage/schemes/generic/homset.py # 2 doctests failed
Modular:
sage t sage/modular/local_comp/local_comp.py # 5 doctests failed sage t sage/modular/abvar/abvar.py # 2 doctests failed sage t sage/modular/hecke/submodule.py # 1 doctest failed sage t sage/modular/abvar/homspace.py # 1 doctest failed sage t sage/modular/modsym/subspace.py # 2 doctests failed sage t sage/modular/hecke/ambient_module.py # 1 doctest failed sage t sage/modular/abvar/finite_subgroup.py # 2 doctests failed sage t sage/modular/modform/cuspidal_submodule.py # 1 doctest failed sage t sage/modular/abvar/homology.py # 2 doctests failed sage t sage/modular/abvar/abvar_newform.py # 1 doctest failed sage t sage/modular/abvar/cuspidal_subgroup.py # 2 doctests failed sage t sage/modular/hecke/degenmap.py # 1 doctest failed sage t sage/modular/modsym/element.py # 1 doctest failed sage t sage/modular/hecke/homspace.py # 1 doctest failed sage t sage/modular/abvar/lseries.py # 2 doctests failed sage t sage/modular/hecke/morphism.py # 1 doctest failed
Rings:
sage t sage/rings/morphism.pyx # 7 doctests failed sage t sage/rings/quotient_ring.py # 1 doctest failed sage t sage/rings/finite_rings/homset.py # 3 doctests failed sage t sage/rings/number_field/morphism.py # 4 doctests failed sage t sage/rings/homset.py # 2 doctests failed
Modules
sage t sage/modules/fg_pid/fgp_morphism.py # 4 doctests failed sage t sage/modules/quotient_module.py # 2 doctests failed sage t sage/modules/vector_space_homspace.py # 1 doctest failed
Others:
sage t sage/doctest/forker.py # 1 doctest failed sage t sage/doctest/control.py # 1 doctest failed sage t sage/structure/parent.pyx # 1 doctest failed sage t sage/categories/hecke_modules.py # 1 doctest failed sage t sage/homology/simplicial_complex_homset.py # 1 doctest failed
Getting failures in sage/doctest seems amazing.
What to do?
comment:4 Changed 8 years ago by
Just a crazy idea (brain storm):
We do not want WithEqualityById
by default, because it makes no sense to have Homset be a unique parent, if domain and codomain are no unique parents.
However, if both domain and codomain are unique parents, it *is* a good idea to use WithEqualityById
.
So, we could check whether both domain and codomain inherit from UniqueRepresentation
. If they do, then they are unique parents, and then we could create a dynamic class out of WithEqualityById
and the class of the homset, and overload the __class__
of the homset.
In that way, we would automatically have fast homset comparison and hash, for those parts of sage that use unique representation.
I am running tests with an according patch now...
comment:5 Changed 8 years ago by
With what I described in my previous post, I get these errors.
sage t sage/schemes/elliptic_curves/padic_lseries.py # 1 doctest failed sage t sage/schemes/elliptic_curves/ell_point.py # 8 doctests failed sage t sage/schemes/generic/homset.py # 1 doctest failed sage t sage/schemes/elliptic_curves/ell_curve_isogeny.py # 8 doctests failed sage t sage/schemes/elliptic_curves/ell_finite_field.py # 1 doctest failed sage t sage/schemes/elliptic_curves/isogeny_class.py # 1 doctest failed sage t sage/schemes/projective/projective_space.py # 2 doctests failed sage t sage/schemes/affine/affine_space.py # 1 doctest failed
Modular:
sage t sage/modular/hecke/morphism.py # 1 doctest failed sage t sage/modular/modsym/element.py # 1 doctest failed sage t sage/modules/quotient_module.py # 2 doctests failed sage t sage/modular/abvar/lseries.py # 2 doctests failed sage t sage/modular/abvar/homology.py # 2 doctests failed sage t sage/modular/abvar/abvar_newform.py # 1 doctest failed sage t sage/modular/abvar/cuspidal_subgroup.py # 2 doctests failed sage t sage/modular/abvar/finite_subgroup.py # 2 doctests failed sage t sage/modular/modform/cuspidal_submodule.py # 1 doctest failed sage t sage/modular/abvar/abvar.py # 2 doctests failed sage t sage/modular/local_comp/local_comp.py # 5 doctests failed sage t sage/modular/hecke/submodule.py # 1 doctest failed sage t sage/modular/abvar/homspace.py # 1 doctest failed sage t sage/modular/modsym/subspace.py # 2 doctests failed sage t sage/modular/hecke/ambient_module.py # 1 doctest failed sage t sage/modular/hecke/degenmap.py # 1 doctest failed
and others:
sage t sage/structure/parent.pyx # 1 doctest failed sage t sage/rings/quotient_ring.py # 1 doctest failed sage t sage/modules/fg_pid/fgp_morphism.py # 3 doctests failed sage t sage/rings/morphism.pyx # 3 doctests failed sage t sage/rings/homset.py # 1 doctest failed
Are these less than before?
comment:6 Changed 8 years ago by
Let's look at the errors more closely:
sage t sage/modular/hecke/morphism.py ********************************************************************** File "sage/modular/hecke/morphism.py", line 110, in sage.modular.hecke.morphism.HeckeModuleMorphism_matrix.__init__ Failed example: t == loads(dumps(t)) Exception raised: Traceback (most recent call last): File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 486, in _run self.execute(example, compiled, test.globs) File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/doctest/forker.py", line 845, in execute exec compiled in globs File "<doctest sage.modular.hecke.morphism.HeckeModuleMorphism_matrix.__init__[2]>", line 1, in <module> t == loads(dumps(t)) File "sage_object.pyx", line 1236, in sage.structure.sage_object.loads (build/cythonized/sage/structure/sage_object.c:11044) File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/homset.py", line 291, in Hom cat_X = X.category() File "module.pyx", line 45, in sage.modules.module.Module_old.category (build/cythonized/sage/modules/module.c:1501) File "classcall_metaclass.pyx", line 330, in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1224) File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/modules.py", line 108, in __classcall_private__ result = super(Modules, cls).__classcall__(cls, base_ring) File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/category.py", line 468, in __classcall__ return super(Category, cls).__classcall__(cls, *args, **options) File "cachefunc.pyx", line 992, in sage.misc.cachefunc.WeakCachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5394) File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/structure/unique_representation.py", line 447, in __classcall__ instance = typecall(cls, *args, **options) File "classcall_metaclass.pyx", line 518, in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:1586) File "/home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/category_types.py", line 326, in __init__ assert base in Rings(), "base must be a ring" AssertionError: <unprintable AssertionError object> ********************************************************************** 1 item had failures: 1 of 4 in sage.modular.hecke.morphism.HeckeModuleMorphism_matrix.__init__ [19 tests, 1 failure, 0.23 s]  sage t sage/modular/hecke/morphism.py # 1 doctest failed  Total time for all tests: 0.3 seconds cpu time: 0.2 seconds cumulative wall time: 0.2 seconds
and this actually looks like it is due to the __reduce__
method, not due to the WithEqualityById
.
comment:7 Changed 8 years ago by
Interesting! This
sage: M = ModularSymbols(6) sage: t = M.Hom(M)(matrix(QQ,3,3,srange(9)), name="spam"); t Hecke module morphism spam defined by the matrix [0 1 2] [3 4 5] [6 7 8] Domain: Modular Symbols space of dimension 3 for Gamma_0(6) of weight ... Codomain: Modular Symbols space of dimension 3 for Gamma_0(6) of weight ... sage: t == loads(dumps(t)) True
works on the command line, but fails in a doctest. :/
comment:8 Changed 8 years ago by
I think I've hit this one before when working on homsets. Perhaps I'll find my old solution somewhere.
comment:9 Changed 8 years ago by
No, I did not find the solution.
The problem seems to be that the modular symbols space M is pickled the *OLD* python way, hence, its __dict__
is pickled. When M is unpickled, some homspace accessible from an attribute of M is unpickled. Codomain and domain of this homspace happen to be M  and if the Hom function is called with an uninitialised version of M, then stuff fails.
Solution: Provide a "proper" pickling for modular symbols. Problem: I don't understand the modular symbols code.
comment:10 Changed 8 years ago by
If I recall correctly, people do explicitly not want to use a cache (such as: UniqueRepresentation
) on modular symbols. They have some cache, but make a point of being able to disable it.
Here is a minimal example, that fails when using a proper __reduce__
method for homsets:
sage: sage.modular.hecke.morphism.is_HeckeModuleMorphism_matrix(ModularSymbols(6).hecke_operator(7).matrix_form().hecke_module_morphism()) True sage: M = ModularSymbols(6) sage: t = M.Hom(M)(matrix(QQ,3,3,srange(9)), name="spam"); t Hecke module morphism spam defined by the matrix [0 1 2] [3 4 5] [6 7 8] Domain: Modular Symbols space of dimension 3 for Gamma_0(6) of weight ... Codomain: Modular Symbols space of dimension 3 for Gamma_0(6) of weight ... sage: t == loads(dumps(t)) True
The first line actually is important. With the first line, we get this error in the last line:
Traceback (most recent call last) <ipythoninput11a4f55210dece> in <module>() > 1 t == loads(dumps(t)) /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/structure/sage_object.so in sage.structure.sage_object.loads (build/cythonized/sage/structure/sage_object.c:11044)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/homset.pyc in Hom(X, Y, category) 289 290 # Determines the category > 291 cat_X = X.category() 292 cat_Y = Y.category() 293 if category is None: /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/modules/module.so in sage.modules.module.Module_old.category (build/cythonized/sage/modules/module.c:1501)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1224)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/modules.pyc in __classcall_private__(cls, base_ring, dispatch) 106 if base_ring in _Fields: 107 return VectorSpaces(base_ring, check=False) > 108 result = super(Modules, cls).__classcall__(cls, base_ring) 109 result._reduction[2]['dispatch'] = False 110 return result /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/category.pyc in __classcall__(cls, *args, **options) 466 if isinstance(cls, DynamicMetaclass): 467 cls = cls.__base__ > 468 return super(Category, cls).__classcall__(cls, *args, **options) 469 470 def __init__(self, s=None): /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/misc/cachefunc.so in sage.misc.cachefunc.WeakCachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5394)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/structure/unique_representation.pyc in __classcall__(cls, *args, **options) 445 446 """ > 447 instance = typecall(cls, *args, **options) 448 assert isinstance( instance, cls ) 449 if instance.__class__.__reduce__ == CachedRepresentation.__reduce__: /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:1586)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/category_types.pyc in __init__(self, base, name) 324 def __init__(self, base, name=None): 325 from sage.categories.rings import Rings > 326 assert base in Rings(), "base must be a ring" 327 Category_over_base.__init__(self, base, name) 328 <type 'str'>: (<type 'exceptions.AttributeError'>, AttributeError('ModularSymbolsAmbient_wt2_g0_with_category' object has no attribute '_HeckeModule_generic__level',))
Hooray, we have a reproducible failure.
comment:11 Changed 8 years ago by
Better:
sage: sage.modular.hecke.morphism.is_HeckeModuleMorphism_matrix(ModularSymbols(6).hecke_operator(7).matrix_form().hecke_module_morphism()) True sage: M = ModularSymbols(6) sage: loads(dumps(M)) is M Traceback (most recent call last) <ipythoninput37b45a005bcab> in <module>() > 1 loads(dumps(M)) is M /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/structure/sage_object.so in sage.structure.sage_object.loads (build/cythonized/sage/structure/sage_object.c:11044)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/homset.pyc in Hom(X, Y, category) 289 290 # Determines the category > 291 cat_X = X.category() 292 cat_Y = Y.category() 293 if category is None: /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/modules/module.so in sage.modules.module.Module_old.category (build/cythonized/sage/modules/module.c:1501)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1224)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/modules.pyc in __classcall_private__(cls, base_ring, dispatch) 106 if base_ring in _Fields: 107 return VectorSpaces(base_ring, check=False) > 108 result = super(Modules, cls).__classcall__(cls, base_ring) 109 result._reduction[2]['dispatch'] = False 110 return result /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/category.pyc in __classcall__(cls, *args, **options) 466 if isinstance(cls, DynamicMetaclass): 467 cls = cls.__base__ > 468 return super(Category, cls).__classcall__(cls, *args, **options) 469 470 def __init__(self, s=None): /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/misc/cachefunc.so in sage.misc.cachefunc.WeakCachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5394)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/structure/unique_representation.pyc in __classcall__(cls, *args, **options) 445 446 """ > 447 instance = typecall(cls, *args, **options) 448 assert isinstance( instance, cls ) 449 if instance.__class__.__reduce__ == CachedRepresentation.__reduce__: /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:1586)() /home/simon/SAGE/prerelease/sage5.10.rc1/local/lib/python2.7/sitepackages/sage/categories/category_types.pyc in __init__(self, base, name) 324 def __init__(self, base, name=None): 325 from sage.categories.rings import Rings > 326 assert base in Rings(), "base must be a ring" 327 Category_over_base.__init__(self, base, name) 328 <type 'str'>: (<type 'exceptions.AttributeError'>, AttributeError('ModularSymbolsAmbient_wt2_g0_with_category' object has no attribute '_HeckeModule_generic__level',))
So, this should be analysable.
comment:12 followup: ↓ 13 Changed 8 years ago by
Here is the problem and a potential solution.
While unpickling of the modular symbols M, we need to construct a homset with domain and codomain M. At this point, calling M.category() results in an error, since M.base() returns None and M.category() wants to return Modules(M.base()).
Hence, in the code of the Hom function from sage.categories.homset, the lines
cat_X = X.category() cat_Y = Y.category()
crash.
The point is: When Hom is called in the process of unpickling a homset, then the correct category is supplied. Hence, the argument "category" of Hom is not None. And I think if in this situation X.category()
raises an error then we can simply ignore the error and skip the consistency check cat_X.is_subcategory(category)
.
Hence, I'd do:
if category is None: category = X.category()._meet_(Y.category()) # Recurse to make sure that Hom(X, Y) and Hom(X, Y, category) are identical H = Hom(X, Y, category) else: # while unpickling, it is possible that the # categories of X and Y are not initialised # and a default category can't be determined. # Since cat_X/Y.is_subcategory(category) is just # a sanity check anyway, we will ignore an error # that is raised when determining cat_X/cat_Y if not isinstance(category, Category): raise TypeError, "Argument category (= %s) must be a category."%category try: cat_X = X.category() except BaseException: cat_X = category try: cat_Y = Y.category() except BaseException: cat_Y = category if not cat_X.is_subcategory(category): raise TypeError, "%s is not in %s"%(X, category) if not cat_Y.is_subcategory(category): raise TypeError, "%s is not in %s"%(Y, category) # Construct H try: # _Hom_ hook from the parent H = X._Hom_(Y, category) ...
Do you think this is a feasible idea?
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 8 years ago by
Replying to SimonKing:
Here is the problem and a potential solution.
While unpickling of the modular symbols M, we need to construct a homset with domain and codomain M. At this point, calling M.category() results in an error, since M.base() returns None and M.category() wants to return Modules(M.base()).
Is None
ever a valid value for M.base()
? At this point, is there enough information available on M
to derive what base()
should return? In that case, I'd think the cleanest way would be to make base()
a caching routine: return a stored value if available and otherwise derive the correct value, store it, and return that. Whenever someone asks for base
they're probably not interested in an invalid value. Or is computing base
possibly expensive and not really necessary for the unpickling?
[...]
Do you think this is a feasible idea?
If you can't easily fix the category determination then, yes. However, what you're proposing is a hack, so solving it properly should really be preferred. (Other people more knowledgeable on category stuff will probably have a more informed opinion than this generic remark)
comment:14 in reply to: ↑ 13 Changed 8 years ago by
Replying to nbruin:
While unpickling of the modular symbols M, we need to construct a homset with domain and codomain M. At this point, calling M.category() results in an error, since M.base() returns None and M.category() wants to return Modules(M.base()).
Is
None
ever a valid value forM.base()
?
I don't know if it is valid, but at least it is easily possible to get None:
sage: Parent().base() is None True
At this point, is there enough information available on
M
to derive whatbase()
should return?
I don't think sounless modular symbols are *always* defined over the rationals. That's not my field of mathematical expertise.
In that case, I'd think the cleanest way would be to make
base()
a caching routine: return a stored value if available and otherwise derive the correct value, store it, and return that. Whenever someone asks forbase
they're probably not interested in an invalid value. Or is computingbase
possibly expensive and not really necessary for the unpickling?
base
is usually set when you call Parent.__init__
.
Anyway. If it is really the case that the base of modular symbols will always be the rational field, then it is fine.
Best regards, Simon
comment:15 followup: ↓ 17 Changed 8 years ago by
Not good. The documentation states:
 ``base_ring``  the base ring. Defaults to `\QQ` if no character is given, or to the minimal extension of `\QQ` containing the values of the character.
I guess the *proper* solution would be to provide a __reduce__
method for modular symbols. Since modular symbols come in many different flavours, this might be a bit awkward.
comment:16 Changed 8 years ago by
Perhaps the following idea works and is clean.
What does Python currently do when unpickling a modular symbol? Well, it creates a new instance of the underlying class, and fills its __dict__
.
I guess it is possible to compute the base ring from the contents of the __dict__
(need to find out how, but I am sure it is encoded there somewhere. Indeed, we have
sage: M = ModularSymbols(6) sage: M._ModularSymbolsSpace__character.base_ring() Rational Field sage: M.base() Rational Field
And that means, we could create an unpickling function doing the following, where dct is the __dict__
and cls is the class of the modular symbol to be unpickled:
M = cls.__new__(cls) ParentWithAdditiveAbelianGens.__init__(M,base=dct['_ModularSymbolsSpace__character'].base_ring(), category=Modules(dct['_ModularSymbolsSpace__character'].base_ring())) M.__dict__ = copy(dct) return M
Well, I will try it later...
comment:17 in reply to: ↑ 15 ; followup: ↓ 18 Changed 8 years ago by
Replying to SimonKing:
An example in the documentation:
sage: G = DirichletGroup(13,GF(4,'a')); sage: e = G.list()[2]; sage: M = ModularSymbols(e,4); sage: M.base() Finite Field in a of size 2^2 sage: M._ModularSymbolsSpace__character.base_ring() Finite Field in a of size 2^2
The base ring can (in principle at least) be pretty much anything, and the way you're proposing to retrieve it seems to work OK (it seems the character is always there)
comment:18 in reply to: ↑ 17 Changed 8 years ago by
Replying to nbruin:
The base ring can (in principle at least) be pretty much anything, and the way you're proposing to retrieve it seems to work OK (it seems the character is always there)
I'm afraid it does not work. The problem is: We need the homset to finish initialisation of the parent, and Python decides to try and create the homset rather early in the reconstruction of the parent. Hence, when we need the character or the base ring or any other information, it is simply not available yet.
I tried to provide a __reduce__
method for HeckeModule_generic
, which solved the problem of pickling modular symbols. But then, a different problem arose: An infinite loop when pickling an abelian variety (to be precise: of ModularAbelianVariety_modsym
). And it is not clear to me how to write a __reduce__
method for abelian varieties such that the loop does not occur.
Probable I should provide the code that I have so far. In about 24 hours, I guess...
And a different approach: We should try to understand why
sage: M = ModularSymbols(6) sage: loads(dumps(M)) == M True
works, while
sage: sage.modular.hecke.morphism.is_HeckeModuleMorphism_matrix(ModularSymbols(6).hecke_operator(7).matrix_form().hecke_module_morphism()) True sage: M = ModularSymbols(6) sage: loads(dumps(M)) == M True
crashes.
comment:19 Changed 8 years ago by
This works:
sage: M = ModularSymbols(6) sage: f = M.hecke_operator(7).matrix_form() sage: loads(dumps(M)) == M True
Continuing the example, this makes it fail:
sage: phi = f.hecke_module_morphism() sage: loads(dumps(M)) == M Traceback (most recent call last): ... <type 'str'>: (<type 'exceptions.AttributeError'>, AttributeError('ModularSymbolsAmbient_wt2_g0_with_category' object has no attribute '_HeckeModule_generic__level',))
Continuing the example, this makes it work again:
sage: del M._HeckeModule_generic__hecke_algebra sage: loads(dumps(M)) == M True
Hence, I think the following happens.
s = dumps(M)
pickles the content ofM.__dict__
.loads(s)
then starts with unpickling the stored content ofM.__dict__
, after creating a wouldbecopy N of M, which is not initialised at this point (this is what Python does in those cases). The unpickled content of M.dict would later be assigned to N.dict The Hecke algebra of M is stored as an attribute of M.
Hom(M,M)
somehow becomes stored as an attribute ofM.hecke_algebra()
when calling.hecke_module_morphism()
. Hence, when deleting the attribute containingM.hecke_algebra()
, all is good.  When unpickling the content of
M.__dict__
, Python eventually tries to unpickleHom(M,M)
, but replaces M with its wouldbecopy N. But since N is not initialised,Hom(N,N)
fails.
This suggests a way out: We could implement "proper" unpickling of M.hecke_algebra()
, so that Hom(M,M)
will not be called prematurely.
It might also be a good idea to use @cached_method when a method is cached. Namely, I often see stuff like
def free_module(self): """ Return the free module underlying this ambient Hecke module (the forgetful functor from Hecke modules to modules over the base ring) EXAMPLE:: sage: ModularForms(59, 2).free_module() Vector space of dimension 6 over Rational Field """ try: return self.__free_module except AttributeError: M = sage.modules.all.FreeModule(self.base_ring(), self.rank()) self.__free_module = M return M
which uses __dict__
and hence will result in errors during unpickling.
comment:20 Changed 8 years ago by
Arrgh. The obvious approach
class HeckeAlgebra_base(sage.rings.commutative_algebra.CommutativeAlgebra): ... def __reduce__(self): return HeckeAlgebra, (self.__M,)
fails, because again for unpickling the Hecke algebra, one needs that M is sufficiently initialised, because otherwise HeckeAlgebra(M)
fails.
comment:21 Changed 8 years ago by
It is a can of worms. The problem will always arise under the following conditions:
 We have
Homset.__reduce__
returningHom, (self._domain, self._codomain, self.__category)
 We have a parent X whose pickling is done via pickling
__dict__
using the default Python way Hom(X,X)
(or any other homset involving X) is stored inX.__dict__
.
In this situation, unpickling X means that we have an uninitialised copy Y of X and want to call Hom(Y,Y).
My previous suggestion was: "In Hom(X,Y,cat), if X.category() fails and cat is given, then trust that X will eventually become sufficiently initialised to see that X is in cat".
Perhaps a modified suggestion is better: "In Hom(X,Y,cat)
, if X.category()
fails, then at least test isinstance(X, cat.parent_class)
." This test is almost as good as X in cat
, since the two tests are actually equivalent, unless a base ring is involved. IF a base ring is involved, then X is in cat up to a change of the base ring.
Do you have a better suggestion? I really would not like to implement __reduce__
methods everywhere in sage.schemes and sage.modular.
comment:22 Changed 8 years ago by
PS: With the idea exposed in the previous post, the following previously failing examples would work:
age: M = ModularSymbols(6) sage: sage.modular.hecke.morphism.is_HeckeModuleMorphism_matrix(ModularSymbols(6).hecke_operator(7).matrix_form().hecke_module_morphism()) True sage: loads(dumps(M)) == M True sage: A = J0(33) sage: D = A.decomposition(); D [ Simple abelian subvariety 11a(1,33) of dimension 1 of J0(33), Simple abelian subvariety 11a(3,33) of dimension 1 of J0(33), Simple abelian subvariety 33a(1,33) of dimension 1 of J0(33) ] sage: loads(dumps(D)) [ Simple abelian subvariety 11a(1,33) of dimension 1 of J0(33), Simple abelian subvariety 11a(3,33) of dimension 1 of J0(33), Simple abelian subvariety 33a(1,33) of dimension 1 of J0(33) ]
comment:23 Changed 8 years ago by
Some thoughts:
 Another plus of my suggestion is that it would encourage people to properly initialise the category (since otherwise X.class would not be a subclass of the category's parent class).
 The negative of my suggestion: If one programs in Cython, then
X.__class__
will not become a subclass of the category's parent class, even if initialisation of the category was done properly.
 The positive in the negative of my suggestion: If one programs in Cython, then the problem is not relevant anyway. The problem I'm addressing will only appear if one relies on Python's way of pickling, without implementing
__reduce__
.
comment:24 Changed 8 years ago by
I have attached an experimental patch that implements the new suggestion. With it, the only error in sage.modular is:
sage t sage/modular/abvar/homspace.py # 1 doctest failed
which is some automatic test suite.
The situation in sage.schemes is still not good, as we get:
sage t sage/schemes/elliptic_curves/ell_curve_isogeny.py # 8 doctests failed sage t sage/schemes/elliptic_curves/ell_point.py # 8 doctests failed sage t sage/schemes/elliptic_curves/ell_finite_field.py # 1 doctest failed sage t sage/schemes/elliptic_curves/isogeny_class.py # 1 doctest failed sage t sage/schemes/projective/projective_space.py # 2 doctests failed sage t sage/schemes/affine/affine_space.py # 1 doctest failed sage t sage/schemes/generic/homset.py # 1 doctest failed
If we are lucky, this boils down to an improper initialisation of the category of some schemes.
comment:25 Changed 8 years ago by
Indeed:
simon@linuxsqwp:~/SAGE/prerelease/sage5.10.rc1/devel/sage> grep "def category" R sage/schemes/ sage/schemes/generic/morphism.py: def category(self):
Overloading CategoryObject.category
smells like an improper use of the category framework in sage.schemes.
comment:26 Changed 8 years ago by
No, it turns out that the reason was different. Schemes create their homsets without using sage.categories.homset.Hom
(instead, a UniqueFactory
is used), and thus one has to overload Homset.__reduce__
to something that uses the custom UniqueFactory
unless someone has the energy to remove this unique factory :)
comment:27 Changed 8 years ago by
Hooray! With the updated patch, all tests in sage.schemes pass!!
So, now I am confident that the general approach will work.
comment:28 Changed 8 years ago by
The only failing test of sage/modular boils down to
sage: E = J0(11).endomorphism_ring() sage: loads(dumps(E))
where again Hom is called on a not sufficiently initialised parent.
Here, the problem is that during unpickling, J0(11).category()
does not raise an error, but returns the category of sets, which is the wrong to do in this situation.
comment:29 Changed 8 years ago by
The patch has been updated again. Now, make ptest almost works:
sage t devel/sage/sage/categories/homset.py # 1 doctest failed sage t devel/sage/sage/modules/fg_pid/fgp_morphism.py # 1 doctest failed
The one failure in homset.py is actually a typo that I introduced in the patch. Hence, trivial to fix.
And the only real error:
sage t sage/modules/fg_pid/fgp_morphism.py ********************************************************************** File "sage/modules/fg_pid/fgp_morphism.py", line 499, in sage.modules.fg_pid.fgp_morphism.FGP_Homset_class._coerce_map_from_ Failed example: phi.parent()._coerce_map_from_(psi.parent()) Expected: True Got: False
To be investigated.
comment:30 Changed 8 years ago by
I just notice that the other one is a real error, too. We want that Hom(X,Y)
is identical with Hom(X1,Y1)
only if X is X1
and Y is Y1
. But in this example, the homsets are identical even though domain and codomain are equal but not identical.
comment:31 Changed 8 years ago by
 Description modified (diff)
 Status changed from new to needs_review
I think I have solved all remaining problems. I needed to modify the suggested logic of "relaxing the category test when there is reason to believe that Hom is called on a domain that is in the process of being unpickled".
It now reads like this:
# Determines the category if category is None: category = X.category()._meet_(Y.category()) # Recurse to make sure that Hom(X, Y) and Hom(X, Y, category) are identical H = Hom(X, Y, category) else: if not isinstance(category, Category): raise TypeError, "Argument category (= %s) must be a category."%category # See trac #14793: It can happen, that Hom(X,X) is called during # unpickling of an instance X of a Python class at a time when # X.__dict__ is empty. In some of these cases, X.category() would # raise a error or would return a too large category (Sets(), for # example) and (worse!) would assign this larger category to the # X._category cdef attribute, so that it would subsequently seem that # X's category was properly initialised. # However, if the above scenario happens, then *before* calling # X.category(), X._is_category_initialised() will correctly say that # it is not initialised. Moreover, since X.__class__ is a Python # class, we will find that `isinstance(X, category.parent_class)`. If # this is the case, then we trust that we indeed are in the process of # unpickling X. Hence, we will trust that `category` has the correct # value, and we will thus skip the test whether `X in category`. try: unpickle_X = (not X._is_category_initialized()) and isinstance(X,category.parent_class) except AttributeError: # this happens for simplicial complexes unpickle_X = False try: unpickle_Y = (not Y._is_category_initialized()) and isinstance(Y,category.parent_class) except AttributeError: unpickle_Y = False if unpickle_X: cat_X = category else: try: cat_X = X.category() except BaseException: raise TypeError, "%s is not in %s"%(X, category) if unpickle_Y: cat_Y = category else: try: cat_Y = Y.category() except BaseException: raise TypeError, "%s is not in %s"%(Y, category)
Note the comment on simplicial complexes: They do have a .category()
method, but they do not derive from CategoryObject
and thus have no ._is_category_initialized()
method. Shame on them.
In addition, some changes needed to be done for SchemeHomsets
and the like. I think one can now start with reviewing.
Apply trac14793homset_default_pickling.patch
comment:32 Changed 8 years ago by
I'm just thinking: Perhaps one should rather define
unpickle_X = hasattr(X, '_is_category_initialized') and (not X._is_category_initialized()) and isinstance(X,category.parent_class)
Changed 8 years ago by
comment:33 Changed 8 years ago by
I needed to replace the old Hom()
method of FGP_Modules
by a _Hom_()
method. While I was at it, I provided their homsets with an element class. However, I did not fix it to the extent that TestSuite(...).run()
would pass.
Apply trac14793homset_default_pickling.patch
comment:34 Changed 8 years ago by
 Cc jpflori added
comment:35 Changed 8 years ago by
Here is an overview of my patch, to facilitate reviewing.
sage.categories.homset
 A homset is provided with inheritance from
WithEqualityById
(which means: Faster comparison), provided that the type of domain and codomain shows that domain and codomain and thus the homset are unique parents.  It must be tested in some way whether domain and codomain of the tobecreated homset belong to the given category. This can fail on parents that are not completely initialised during unpickling. If this parent is an instance of a Python class, then by virtue of the category framework its class tells whether it belongs to the correct category, up to the choice of a base ring. I think that in this case we can assume that the parent will be fine as soon as its reconstruction/unpickling is complete.
 Homsets should be pickled using the
Hom
function, hence, with a cache.
sage.modular.abvar.abvar
 A category should be accepted when creating an endomorphism ring of a variety X. Previously, the correct category was read off of X.category(). However, during unpickling, X.category() might not work, and thus the category should be explicit part of the pickle data (and must hence be passed to the construction of the endomorphism ring).
sage.modular.abvar.homspace
 Pass the category parameter to the construction of homspaces (see above)
 Provide homspaces with an element class
 Use the element class for constructing its elements.
 The
_matrix_space
attribute used to be defined during initialisation, with information obtained from domain and codomain. Again, during unpickling, domain and codomain may be unable to provide such information. Hence, I turn_matrix_space
into a lazy attribute, that will only be called after initialisation, hence, after finishing unpickling of domain and codomain.
sage.modules.fg_pid.fgp_module
 The modules should better rely on the default implementation of the
.Hom()
method. Hence, I renamed.Hom()
into._Hom_()
.
sage.modules.fg_pid.fgp_morphism
 The category should be passed when calling
Homset.__init__
 I provided the homsets with an element class
sage.modules.matrix_morphism
 The constructor of a matrix morphism should accept another matrix morphism as an input. This is to make conversion of matrix morphisms (between distinct but equal homsets) possible. Aim: Let
TestSuite
pass to greater extent.  TODO: To make
TestSuite
fully pass, it would be needed that the morphisms have a_mul_
(single underscore) method. Currently, they only have a__mul__
(double underscore) method. This shall be topic of a ticket that implements the coercion framework for homsets (I created such ticket already, but don't recall the number).
sage.schemes.generic.homset
 I believe that
SchemeHomset
should not rely on aUniqueFactory
, but should rather rely on the cache of theHom
function. However, I did not remove the factory. Instead, I overload the__reduce__
method inherited fromHomset
and use theSchemeHomset
factory for pickling.
I hope this explanation makes the reviewer happy!
comment:36 Changed 8 years ago by
 Branch set to public/structure/unique_repr_homsets14793
 Commit set to ea3ecfbc644fa6b53441c25cce73720f9a0ffeee
 Description modified (diff)
 Reviewers set to Travis Scrimshaw
comment:37 followup: ↓ 39 Changed 8 years ago by
Travis, how difficult has it been to apply the old patch? Certainly there have been a bunch of conflicts.
comment:38 Changed 8 years ago by
For the record: I am happy with your changes, Travis. To be on the safe side, I am running tests on my laptop now, but after all you are the reviewer... And sorry that I forgot to add tests for some new methods.
comment:39 in reply to: ↑ 37 ; followup: ↓ 40 Changed 8 years ago by
Replying to SimonKing:
Travis, how difficult has it been to apply the old patch? Certainly there have been a bunch of conflicts.
None; there were no conflicts. Although it might conflict with #10963..... *gulp*
Actually, I noticed that I missed an added __reduce__()
in categories/homset.py
 it has a docstring but no doctests. Could you add one since I won't have access to a computer with Sage for another 68 hours (I will do it then if you haven't done it)? Thanks.
comment:40 in reply to: ↑ 39 Changed 8 years ago by
Replying to tscrim:
Actually, I noticed that I missed an added
__reduce__()
incategories/homset.py
 it has a docstring but no doctests. Could you add one since I won't have access to a computer with Sage for another 68 hours (I will do it then if you haven't done it)? Thanks.
I see your post only nowand 8 hours are past :)
Anyway, I am adding a test for the __reduce__
method now. We should then have another look at the "patch" and see what hasn't been tested.
comment:41 Changed 8 years ago by
 Commit changed from ea3ecfbc644fa6b53441c25cce73720f9a0ffeee to e2d2f167429b625b12690f91d1d8d69aaf69f91b
Branch pushed to git repo; I updated commit sha1. New commits:
e2d2f16  Trac 14793: Add a missing doctest

comment:42 Changed 8 years ago by
Test added.
comment:43 Changed 8 years ago by
PS: I verified that the homsets in the test really use this and no other method for pickling.
comment:44 Changed 8 years ago by
Good news: #10963 merges cleanly.
comment:45 Changed 8 years ago by
comment:46 Changed 8 years ago by
 Commit changed from e2d2f167429b625b12690f91d1d8d69aaf69f91b to 17fe8ee3ed896c7f6ffb83e002251e7087ef1df3
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
17fe8ee  Started transition.

0ae3289  Initial attempt at _internal_coerce_map_from_() method.

45597e1  Merge branch 'u/jpflori/ticket/14711' of trac.sagemath.org:sage into public/structure/unique_repr_homsets14793

0af59ea  Reviewer changes. Mostly formatting.

37bf59e  Merge branch 'develop' into ticket/14711, resolving conflicts with Trac 12217

ee30c20  Address the "check" keyword of scheme morphisms by name, not position

d68c5df  Minor fixes, that became needed since #14711 was not merged quickly enough

c42b539  Merge branch 'master' into ticket/14711

23f18f2  Merge branch 'master' into ticket/14711

364b985  Add warning to string repr of weakened maps. Implement copying for *all* kinds of maps.

comment:47 Changed 8 years ago by
 Commit changed from 17fe8ee3ed896c7f6ffb83e002251e7087ef1df3 to 0af59ea93689cb6abb9d3fae0f1cf11f2aee5cca
Branch pushed to git repo; I updated commit sha1. This was a forced push. Recent commits:
0af59ea  Reviewer changes. Mostly formatting.

37bf59e  Merge branch 'develop' into ticket/14711, resolving conflicts with Trac 12217

ee30c20  Address the "check" keyword of scheme morphisms by name, not position

d68c5df  Minor fixes, that became needed since #14711 was not merged quickly enough

c42b539  Merge branch 'master' into ticket/14711

comment:48 Changed 8 years ago by
 Commit changed from 0af59ea93689cb6abb9d3fae0f1cf11f2aee5cca to e2d2f167429b625b12690f91d1d8d69aaf69f91b
comment:49 followup: ↓ 50 Changed 8 years ago by
I'm an idiot and merged the wrong branches...
comment:50 in reply to: ↑ 49 Changed 8 years ago by
Replying to tscrim:
I'm an idiot and merged the wrong branches...
Doesn't this qualify as "changing history"? You bad boy ;)
comment:51 Changed 8 years ago by
It's not changing history, just taking a different path :p
.
comment:52 followup: ↓ 53 Changed 8 years ago by
I think we're good to go here unless there's something else you can see or think of?
comment:53 in reply to: ↑ 52 ; followup: ↓ 54 Changed 8 years ago by
Replying to tscrim:
I think we're good to go here unless there's something else you can see or think of?
Just to make sure: We have three commits, namely one that corresponds to the original patch, your review changes, and then my commit adding one doctest?
For the record, I agree with your review changes. But after all, you are the reviewer, not I. If you think the code is fine and if all tests pass after merging the develop branch, I'd not oppose to let this be positively reviewed.
comment:54 in reply to: ↑ 53 Changed 8 years ago by
Replying to SimonKing:
Just to make sure: We have three commits, namely one that corresponds to the original patch, your review changes, and then my commit adding one doctest?
Correct.
For the record, I agree with your review changes. But after all, you are the reviewer, not I. If you think the code is fine and if all tests pass after merging the develop branch, I'd not oppose to let this be positively reviewed.
I think everything is good, so positive review. Thank you for your work on this Simon. Now #14279 and its dependency (after we finish #10963 and the weak coercions).
comment:55 Changed 8 years ago by
 Status changed from needs_review to positive_review
comment:56 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:57 Changed 7 years ago by
 Branch changed from public/structure/unique_repr_homsets14793 to e2d2f167429b625b12690f91d1d8d69aaf69f91b
 Resolution set to fixed
 Status changed from positive_review to closed
To discuss: the exact semantic for homsets of non unique parents