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: sage-6.2
Component: categories Keywords:
Cc: sage-combinat, 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:

Status badges

Description (last modified by tscrim)

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)

homset_default_pickling.patch (10.9 KB) - added by SimonKing 8 years ago.
Experimental patch
trac14793-homset_default_pickling.patch (15.3 KB) - added by SimonKing 8 years ago.

Download all attachments as: .zip

Change History (59)

comment:1 Changed 8 years ago by nthiery

  • Cc SimonKing added

To discuss: the exact semantic for homsets of non unique parents

comment:2 Changed 8 years ago by SimonKing

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 SimonKing

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 SimonKing

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 SimonKing

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 SimonKing

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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 486, in _run
        self.execute(example, compiled, test.globs)
      File "/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/categories/modules.py", line 108, in __classcall_private__
        result = super(Modules, cls).__classcall__(cls, base_ring)
      File "/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/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 SimonKing

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 SimonKing

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 SimonKing

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 SimonKing

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)
<ipython-input-11-a4f55210dece> in <module>()
----> 1 t == loads(dumps(t))

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.loads (build/cythonized/sage/structure/sage_object.c:11044)()

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/modules/module.so in sage.modules.module.Module_old.category (build/cythonized/sage/modules/module.c:1501)()

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1224)()                                                                                                                                                                                 

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/misc/cachefunc.so in sage.misc.cachefunc.WeakCachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5394)()

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:1586)()                                                                                                                                                                                                    

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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 SimonKing

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)
<ipython-input-3-7b45a005bcab> in <module>()
----> 1 loads(dumps(M)) is M

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.loads (build/cythonized/sage/structure/sage_object.c:11044)()

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/modules/module.so in sage.modules.module.Module_old.category (build/cythonized/sage/modules/module.c:1501)()

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (build/cythonized/sage/misc/classcall_metaclass.c:1224)()                                                                                                                                                                                 

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/misc/cachefunc.so in sage.misc.cachefunc.WeakCachedFunction.__call__ (build/cythonized/sage/misc/cachefunc.c:5394)()

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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/sage-5.10.rc1/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.typecall (build/cythonized/sage/misc/classcall_metaclass.c:1586)()                                                                                                                                                                                                    

/home/simon/SAGE/prerelease/sage-5.10.rc1/local/lib/python2.7/site-packages/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 follow-up: Changed 8 years ago by 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()).

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 ; follow-up: Changed 8 years ago by nbruin

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 SimonKing

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 for M.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 what base() should return?

I don't think so---unless 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 for base they're probably not interested in an invalid value. Or is computing base 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 follow-up: Changed 8 years ago by SimonKing

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 SimonKing

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 ; follow-up: Changed 8 years ago by nbruin

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 SimonKing

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 SimonKing

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 of M.__dict__. loads(s) then starts with unpickling the stored content of M.__dict__, after creating a would-be-copy 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 of M.hecke_algebra() when calling .hecke_module_morphism(). Hence, when deleting the attribute containing M.hecke_algebra(), all is good.
  • When unpickling the content of M.__dict__, Python eventually tries to unpickle Hom(M,M), but replaces M with its would-be-copy 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 SimonKing

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 SimonKing

It is a can of worms. The problem will always arise under the following conditions:

  • We have Homset.__reduce__ returning Hom, (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 in X.__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.

Last edited 8 years ago by SimonKing (previous) (diff)

comment:22 Changed 8 years ago by SimonKing

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 SimonKing

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 sub-class 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 SimonKing

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 SimonKing

Indeed:

simon@linux-sqwp:~/SAGE/prerelease/sage-5.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 SimonKing

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 SimonKing

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 SimonKing

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.

Changed 8 years ago by SimonKing

Experimental patch

comment:29 Changed 8 years ago by SimonKing

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 SimonKing

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 SimonKing

  • 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 trac14793-homset_default_pickling.patch

comment:32 Changed 8 years ago by SimonKing

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 SimonKing

comment:33 Changed 8 years ago by SimonKing

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 trac14793-homset_default_pickling.patch

comment:34 Changed 8 years ago by jpflori

  • Cc jpflori added

comment:35 Changed 8 years ago by SimonKing

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 to-be-created 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 a UniqueFactory, but should rather rely on the cache of the Hom function. However, I did not remove the factory. Instead, I overload the __reduce__ method inherited from Homset and use the SchemeHomset factory for pickling.

I hope this explanation makes the reviewer happy!

Last edited 8 years ago by SimonKing (previous) (diff)

comment:36 Changed 8 years ago by tscrim

  • Branch set to public/structure/unique_repr_homsets-14793
  • Commit set to ea3ecfbc644fa6b53441c25cce73720f9a0ffeee
  • Description modified (diff)
  • Reviewers set to Travis Scrimshaw

Made this into a branch and added doctests to added functions and some other misc review changes. If you're happy with my changes Simon, then this is a positive review.


New commits:

ea3ecfbSome review changes.
1411319#14793: Use __reduce__ for pickling homsets, and use WithEqualityById for them

comment:37 follow-up: Changed 8 years ago by SimonKing

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 SimonKing

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 ; follow-up: Changed 8 years ago by tscrim

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 6-8 hours (I will do it then if you haven't done it)? Thanks.

comment:40 in reply to: ↑ 39 Changed 8 years ago by SimonKing

Replying to tscrim:

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 6-8 hours (I will do it then if you haven't done it)? Thanks.

I see your post only now---and 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 git

  • Commit changed from ea3ecfbc644fa6b53441c25cce73720f9a0ffeee to e2d2f167429b625b12690f91d1d8d69aaf69f91b

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

e2d2f16Trac 14793: Add a missing doctest

comment:42 Changed 8 years ago by SimonKing

Test added.

comment:43 Changed 8 years ago by SimonKing

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 SimonKing

Good news: #10963 merges cleanly.

comment:45 Changed 8 years ago by tscrim

I put sleep > sage last night, so :p and thanks for adding that. It's also good to know that there are no conflicts with #10963. I've gone through your changes, tests pass, and everything is documented. The TestSuite and things is #14279. What else is left here to review?

comment:46 Changed 8 years ago by git

  • Commit changed from e2d2f167429b625b12690f91d1d8d69aaf69f91b to 17fe8ee3ed896c7f6ffb83e002251e7087ef1df3

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

17fe8eeStarted transition.
0ae3289Initial attempt at _internal_coerce_map_from_() method.
45597e1Merge branch 'u/jpflori/ticket/14711' of trac.sagemath.org:sage into public/structure/unique_repr_homsets-14793
0af59eaReviewer changes. Mostly formatting.
37bf59eMerge branch 'develop' into ticket/14711, resolving conflicts with Trac 12217
ee30c20Address the "check" keyword of scheme morphisms by name, not position
d68c5dfMinor fixes, that became needed since #14711 was not merged quickly enough
c42b539Merge branch 'master' into ticket/14711
23f18f2Merge branch 'master' into ticket/14711
364b985Add warning to string repr of weakened maps. Implement copying for *all* kinds of maps.

comment:47 Changed 8 years ago by git

  • Commit changed from 17fe8ee3ed896c7f6ffb83e002251e7087ef1df3 to 0af59ea93689cb6abb9d3fae0f1cf11f2aee5cca

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

0af59eaReviewer changes. Mostly formatting.
37bf59eMerge branch 'develop' into ticket/14711, resolving conflicts with Trac 12217
ee30c20Address the "check" keyword of scheme morphisms by name, not position
d68c5dfMinor fixes, that became needed since #14711 was not merged quickly enough
c42b539Merge branch 'master' into ticket/14711

comment:48 Changed 8 years ago by git

  • Commit changed from 0af59ea93689cb6abb9d3fae0f1cf11f2aee5cca to e2d2f167429b625b12690f91d1d8d69aaf69f91b

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

e2d2f16Trac 14793: Add a missing doctest
ea3ecfbSome review changes.
1411319#14793: Use __reduce__ for pickling homsets, and use WithEqualityById for them

comment:49 follow-up: Changed 8 years ago by tscrim

I'm an idiot and merged the wrong branches...

comment:50 in reply to: ↑ 49 Changed 8 years ago by SimonKing

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 tscrim

It's not changing history, just taking a different path :p.

comment:52 follow-up: Changed 8 years ago by tscrim

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 ; follow-up: Changed 8 years ago by SimonKing

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 tscrim

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 tscrim

  • Status changed from needs_review to positive_review

comment:56 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:57 Changed 7 years ago by vbraun

  • Branch changed from public/structure/unique_repr_homsets-14793 to e2d2f167429b625b12690f91d1d8d69aaf69f91b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.