Hom: introduce a check argument to simplify the unpickling detection logic

[nthiery]

As was noticed in #14793, some sanity checks need to be disabled in Hom when called upon unpickling because the input may include an unitialized parent. Since #14793, this is achieved by looking at the parent to detect if it is unitilialized.

As an alternative, this ticket proposes to add a check=True/False
optional argument to Hom, and to use it upon unpickling. The advantages are:

• The logic is quite simpler, slightly faster, and more robust.

• This check argument is of general purpose, and indeed immediately
  put to use when Hom calls itself recursively.

• It made it easier for the first feature below

A potential caveat: pickles created since #14793 might not unpickle
properly. This was very recent; do we care?

Other changes in this ticket:

• Hom now uses X in category as sanity check rather than
  X.category().is_subcategory(category). This is more expressive,
  and indeed a necessary preliminary step for Categories over a base ring category #15801 which makes those
  two idioms not always equivalent (example: X is a QQ-module,
  X.category() is Modules(Rings) and category is
  Modules(QQ))

• A bug fix for Homsets between simplicial complexes over some higher
  category (Hom was not checking its input).

CC: @simon-king-jena @tscrim

Component: categories

Keywords: homset

Author: Nicolas M. Thiéry, Simon King

Branch: e1e916c

Reviewer: Simon King, Nicolas M. Thiéry

Issue created by migration from https://trac.sagemath.org/ticket/16275

[nthiery]

Branch: u/nthiery/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic

[sagetrac-git]

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

1ff993b

#16275: trivial commit to use format

[sagetrac-git]

Commit: 1ff993b

[tscrim]

comment:3

I agree with these changes, although I don't understand pickling well enough to answer those issues. Simon, could you render judgement on this or clarify the issue?

[nthiery]

comment:4

Just in case this helps, here is an extract of how quotients of
multivariate polynomial rings are unpickled. We see in the
construction of si8 Hom being called on the parents si5 and si7, where
si5 (the resulting quotient ring) has been allocated by
unpickle_newobj, but not yet initialized by unpickle_build:

    sage: P.<x,y> = QQ['x,y']
    sage: Q = P.quotient([x^2-1,y^2-1])
    sage: Q.an_element()
    sage: explain_pickle(dumps(Q))
    ...
    si4 = pg_dynamic_class('QuotientRing_generic_with_category', (pg_QuotientRing_generic, pg_getattr(si3, 'parent_class')), None, None, pg_QuotientRing_generic)
    si5 = unpickle_newobj(si4, ())
    ...
    pg_Hom = unpickle_global('sage.categories.homset', 'Hom')
    pg_unpickle_MPolynomialRing_libsingular = unpickle_global('sage.rings.polynomial.multi_polynomial_libsingular', 'unpickle_MPolynomialRing_libsingular')
    ...
    si7 = pg_unpickle_MPolynomialRing_libsingular(pg1, ('x', 'y'), si6)
    ...
    si8 = pg_unpickle_map(pg_RingMap_lift, pg_Hom(si5, si7, pg_unreduce(pg_Sets, (), {}), False), {}, {'_codomain':si7, 'S':si7, '_domain':si5, '_repr_type_str':None})
    ...
    unpickle_build(si5, {'_QuotientRing_nc__lift':si8, '_embedding':None, '_initial_action_list':[], '_convert_method_name':None, '_QuotientRing_nc__cover':si9, '_category':si3, '_names':('xbar', 'ybar'), 'cover':pg_CachedMethodPickle(si5, 'cover', si9), 'lifting_map':pg_CachedMethodPickle(si5, 'lifting_map', si8), '_QuotientRing_nc__R':si7, 'element_class':pg_dynamic_class('QuotientRing_generic_with_category.element_class', (pg_QuotientRingElement, pg_getattr(si3, 'element_class')), None, None, None), '_generators':{}, '_QuotientRing_nc__I':si10, '_base':pg1, '_pickle_version':1r, '_element_constructor':pg_unpickleMethod('_element_constructor_', si5, si4), '_initial_convert_list':[], '_element_init_pass_parent':False, '_cdata':None, '_initial_coerce_list':[]})
    si5

[sagetrac-git]

Changed commit from 1ff993b to 0074f02

[sagetrac-git]

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

0074f02

#16275: added doctest illustrating an unpickling where Hom is called on an uninitialized parent

[sagetrac-git]

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

c1e91f3	#16275: update ticket number in the comments
ae3ca80	This is handled as well by the generic of Homset.__reduce__ method,

[sagetrac-git]

Changed commit from 0074f02 to ae3ca80

[nthiery]

comment:7

With the latest commit, all tests pass. Running the long tests now.

[simon-king-jena]

Replying to @nthiery:

As an alternative, this ticket proposes to add a check=True/False
optional argument to Hom, and to use it upon unpickling.

I agree that this is a good approach.

A potential caveat: pickles created since #14793 might not unpickle
properly. This was very recent; do we care?

I would prefer if we could preserve these potential pickles. Perhaps it is possible to have a "try-except" block somewhere, and unpickle the old way only if the new way fails. This would be without a noticeable speed regression, I hope.

Note, in any case, that post-#14793 pickles that can not be unpickled with your changes could not be unpickled pre-#14793 either.

Other changes in this ticket:

• Hom now uses X in category as sanity check rather than
  X.category().is_subcategory(category).

+1

I will try to review it after lunch. Certainly there has to be a review patch, e.g., in order to remove the old __reduce__ method that you left in under the name __reduce__disabled__.

[simon-king-jena]

comment:9

Replying to @tscrim:

I agree with these changes, although I don't understand pickling well enough to answer those issues. Simon, could you render judgement on this or clarify the issue?

Let's try...

The problem occurs if Python's default way of pickling/copying is used: Assume that you have an object O, pickle it, and want to unpickle it into a new object N.

When pickling O in Python's default way, type(O) and O.__dict__ are stored. When unpickling, N is created as a new instance of type(O), and N.__dict__ is filled by unpickling the pickle of O.__dict__. Of course, when unpickling O.__dict__, N takes the role of O.

The problem: It could be that you need N.__dict__['foo'] in order to unpickle N.__dict__['bar']. To be more concrete, it could be that some morphism phi from O to somewhere else is stored as an attribute O.foo. Hence, to unpickle phi, you also need to unpickle some Hom(O,P,C) for some category C. So, Hom(N,P,C) will be created. But at that point, N.__dict__ is not completely filled. Perhaps N can not tell whether it is an object of C before N.bar is available. Thus, some assertion during creation of the homset will fail.

The obvious approach towards a solution: When we have a pickle, then the assertions need not be made, as we know that the to-be-unpickled object belongs to the category of the to-be-unpickled homset.

In #14793, it was suggested to skip the assertion if the category of domain or codomain is not initialised, since this indicates that we are in the process of unpickling domain and/or codomain.

Here, it is suggested to be more explicit and have an optional argument for skipping the assertion.

I think explicit is better than implicit. Moreover, it has another use case (as demonstrated by Nicolas) and should also be faster than the old solution. Hence, I am all in favour of the new solution.

The only problem is: (How) Do we care about old pickles whose unpickling requires to skip the assertion, but which are unaware of the "check=False" argument?

I suggest the following scheme:

    if check:
        if domain not in category:
            if domain's category is initialised:
                raise error, since that means we are not unpickling an old pickle
        if codomain not in category:
            if codomain's category is initialised:
                raise error
    proceed with creating the homset

Hence, the assertion is skipped if this is explicitly requested, and before raising an actual error it is verified if we need to skip the assertion implicitly.

[simon-king-jena]

comment:10

With the current branch, sage -t src/sage/modular/abvar/homspace.py fails:

sage -t src/sage/modular/abvar/homspace.py
**********************************************************************
File "src/sage/modular/abvar/homspace.py", line 762, in sage.modular.abvar.homspace.EndomorphismSubring.__init__
Failed example:
    E.__reduce__()
Expected:
    (<function Hom at ...>,
     (Abelian variety J0(11) of dimension 2,
      Abelian variety J0(11) of dimension 2,
      Category of modular abelian varieties over Rational Field,
     False))
Got:
    (<function Hom at 0x9793dbc>, (Abelian variety J0(11) of dimension 1, Abelian variety J0(11) of dimension 1, Category of modular abelian varieties over Rational Field, False))

Hopefully this is just a trivial typo (2 instead of 1). I'll try to fix that in my review patch.

[simon-king-jena]

A failing pickle

[simon-king-jena]

comment:11

Attachment: broken_pickle.sobj.gz

I have attached a pickle that can't be unpickled with the current branch.

I suggest that we make it so that it can still be unpickled (as I have sketched in comment:9) and add it to the pickle jar. Can you tell me how to add stuff to the pickle jar?

[simon-king-jena]

comment:12

I have added it to the picklejar, but now I don't know how to post the change. It seems that the picklejar is not under version control.

Anyway, I think we need to catch errors properly, and raise a more informative error. What happens with the pickle is this:

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.load (sage/structure/sage_object.c:9774)()

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.loads (sage/structure/sage_object.c:11522)()

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/categories/homset.pyc in Hom(X, Y, category, check)
    374                 raise TypeError("Argument category (= {}) must be a category.".format(category))
    375             for O in [X, Y]:
--> 376                 if O not in category:
    377                     raise TypeError("{} is not in {}".format(O, category))
    378 

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/categories/category.pyc in __contains__(self, x)
    722         """
    723         try:
--> 724             c = x.category()
    725         except AttributeError:
    726             return False

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/modules/module.so in sage.modules.module.Module_old.category (sage/modules/module.c:1604)()

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.ClasscallMetaclass.__call__ (sage/misc/classcall_metaclass.c:1282)()

/home/king/Sage/git/sage/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/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/categories/category.pyc in __classcall__(cls, *args, **options)
    462         if isinstance(cls, DynamicMetaclass):
    463             cls = cls.__base__
--> 464         return super(Category, cls).__classcall__(cls, *args, **options)
    465 
    466     def __init__(self, s=None):

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/cachefunc.so in sage.misc.cachefunc.WeakCachedFunction.__call__ (sage/misc/cachefunc.c:6486)()

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/structure/unique_representation.pyc in __classcall__(cls, *args, **options)
   1019             True
   1020         """
-> 1021         instance = typecall(cls, *args, **options)
   1022         assert isinstance( instance, cls )
   1023         if instance.__class__.__reduce__ == CachedRepresentation.__reduce__:

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/misc/classcall_metaclass.so in sage.misc.classcall_metaclass.typecall (sage/misc/classcall_metaclass.c:1665)()

/home/king/Sage/git/sage/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, Nicolas' nicely formatted error is not seen, and a rather mysterious error about a missing attribute appears.

Again, my suggestion is to catch this error and see if it comes from incomplete initialisation.

[simon-king-jena]

comment:13

Argh, it is even funnier: The attribute error is raised when trying to format the error message! So, for a nicer error message, we need to have a try-except around the creation of a string representation.

[simon-king-jena]

comment:14

Even stranger, I see the following:

sage: t = load("/home/king/Downloads/broken_pickle.sobj")
have error message <class 'sage.modular.modsym.ambient.ModularSymbolsAmbient_wt2_g0_with_category'> instance is not in Category of Hecke modules over Rational Field
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-540ff1d7475d> in <module>()
----> 1 t = load("/home/king/Downloads/broken_pickle.sobj")

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.load (sage/structure/sage_object.c:9774)()

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/structure/sage_object.so in sage.structure.sage_object.loads (sage/structure/sage_object.c:11522)()

/home/king/Sage/git/sage/local/lib/python2.7/site-packages/sage/categories/homset.pyc in Hom(X, Y, category, check)
    387                         error_msg = "{} instance is not in {}".format(type(O), category)
    388                     print "have error message",error_msg
--> 389                     raise ValueError(error_msg)
    390 
    391         # Construct H

<type 'str'>: (<type 'exceptions.AttributeError'>, AttributeError('ModularSymbolsAmbient_wt2_g0_with_category' object has no attribute '_HeckeModule_generic__level',))

I think a ValueError should be raised, not a TypeError. Anyway, a ValueError is raised, with a sufficiently nice error message, but a totally different error is shown to the user!

Bug in ipython??

[nthiery]

comment:15

Replying to @simon-king-jena:

With the current branch, sage -t src/sage/modular/abvar/homspace.py fails:

sage -t src/sage/modular/abvar/homspace.py
**********************************************************************
File "src/sage/modular/abvar/homspace.py", line 762, in sage.modular.abvar.homspace.EndomorphismSubring.__init__
Failed example:
    E.__reduce__()
Expected:
    (<function Hom at ...>,
     (Abelian variety J0(11) of dimension 2,
      Abelian variety J0(11) of dimension 2,
      Category of modular abelian varieties over Rational Field,
     False))
Got:
    (<function Hom at 0x9793dbc>, (Abelian variety J0(11) of dimension 1, Abelian variety J0(11) of dimension 1, Category of modular abelian varieties over Rational Field, False))

Hopefully this is just a trivial typo (2 instead of 1). I'll try to fix that in my review patch.

Oops, yes, just a typo. When moving around the example, I changed the J0(31) to E=J0(11), and forgot to update the doctest. Thanks.

[nthiery]

comment:16

Replying to @simon-king-jena:

I think explicit is better than implicit. Moreover, it has another use case (as demonstrated by Nicolas) and should also be faster than the old solution. Hence, I am all in favour of the new solution.

Glad we are on the same line :-) And thanks for checking and investigating further!

The only problem is: (How) Do we care about old pickles whose unpickling requires to skip the assertion, but which are unaware of the "check=False" argument?

I suggest the following scheme:

    if check:
        if domain not in category:
            if domain's category is initialised:
                raise error, since that means we are not unpickling an old pickle
        if codomain not in category:
            if codomain's category is initialised:
                raise error
    proceed with creating the homset

Hence, the assertion is skipped if this is explicitly requested, and before raising an actual error it is verified if we need to skip the assertion implicitly.

One thing: domain not in category will possibly call
domain.category() with the caveats that were mentioned on #14793.
Not sure how much we should care.

As to how important it is to actually do something about this: what's
the time period during which pickles may have been created that can't
be unpickled with the check=False approach?

Cheers,
Nicolas

[nthiery]

comment:17

For the record, up to the trivial failing test noted in [comment:10], all long tests passed.

[simon-king-jena]

comment:18

Replying to @nthiery:

One thing: domain not in category will possibly call
domain.category() with the caveats that were mentioned on #14793.

Indeed, that's part of the problem with the broken pickle :attachment:broken_pickle.sobj

As to how important it is to actually do something about this: what's
the time period during which pickles may have been created that can't
be unpickled with the check=False approach?

No idea. But if we find a way to unpickle the broken pickle without slowing down the mainline, then I'd say we should provide it.

[simon-king-jena]

comment:19

Replying to @simon-king-jena:

Replying to @nthiery:

One thing: domain not in category will possibly call
domain.category() with the caveats that were mentioned on #14793.

Indeed, that's part of the problem with the broken pickle :attachment:broken_pickle.sobj

No, it isn't. The caveat was that calling domain.category() stores the returned category, and it may then be the case that domain._is_category_initialized() returns True, even though the category was too large.

In any case, when we only raise an error if the category does not match and O._is_category_initialized() is true, then the previously broken pickle can be unpickled.

So, question: How can I post the updated picklejar?

[sagetrac-git]

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

507ee00

Trac 16275: fixes for the previous commit; btw its title should have read: removed sage.modular.abvar.homspace.Homspace.__reduce__

[sagetrac-git]

Changed commit from ae3ca80 to 507ee00

[simon-king-jena]

comment:35

sage.homology.simplicial_complex is a mess, category-wise. It provides a method category(), but it should instead inherit from CategoryObject.

Let me see how difficult it will be to fix.

[simon-king-jena]

comment:36

I suggest that GenericCellComplex keeps being derived from SageObject, since it has no functional category() method anyway. However, CategoryObject should be added as new base class to SimplicialComplex, and it should be appropriately initialised.

Doing so makes TestSuite(...).run() pass on simplicial complexes. Hence, it seems doable to fix this here. What do you think?

[sagetrac-git]

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

e1e916c

Simplicial complexes are now CategoryObject. Switch TypeError into ValueError

[sagetrac-git]

Changed commit from ecc4616 to e1e916c

[simon-king-jena]

comment:38

It seems to work. At least: Tests of sage.categories.homset and sage.homology all pass. Since I actually changed code, I am promoting myself to an "Author"...

So, see if you like the changes (and promote yourself to a "Reviewer"...). Provided that the full test suite passes, which I will be testing later.

[simon-king-jena]

Changed author from Nicolas M. Thiéry to Nicolas M. Thiéry, Simon King

[nthiery]

comment:39

Replying to @simon-king-jena:

Really? This sounds ugly to me. And the purpose of the pickle jar is not "go search for the potential problem". IF there will be a problem in future then the pickle jar will inform us.

It will inform us indeed. But if as a developer I wonder why all this
logic is needed, and want to experiment on a concrete example, I need to dig
in the jar to find the pickle that demonstrates it. In short, having the pickle in the doctest would keep the test close to the code.

But I don't have a strong opinion about that either.

But yes, I agree, it's a bit cryptic. And it's not (yet?) common usage even though it's not unprecedented either (see __setstate__ in combinat/partition.py or combinat/ribbon_shaped_tableau.py).

Ahem, I clearly wrote See trac #16275 and #14793.

There was a race condition; I still was on the previous commit :-)

Cheers,

[nthiery]

Changed reviewer from Simon King to Simon King, Nicolas M. Thiéry

[nthiery]

comment:40

I am happy with the change making SimplicialComplex a CategoryObject. So I guess this is positive review once the pickle jar thing is settled and it's confirmed that all tests pass.

[simon-king-jena]

comment:41

make ptest
...
----------------------------------------------------------------------
All tests passed!
----------------------------------------------------------------------
Total time for all tests: 4161.3 seconds
    cpu time: 6208.2 seconds
    cumulative wall time: 7651.2 seconds

This is with the (locally) updated pickle jar.

Concerning pickle jar, I asked on sage-devel how to add to the pickle jar (in the sense of "how to post changes to a trac ticket, as the pickle jar is not under version control"). There has been no answer yet.

[nthiery]

comment:42

Cool!

Coming back to the idea of putting the pickle as a doctest; there is a
quite natural way to avoid the ugly meaningless string: to put instead
the corresponding sequence of instructions, as given by
explain_pickle.

Hmm, ok, maybe that's a bit too long in this case :-)

sage: pg_unpickle_map = unpickle_global('sage.categories.map', 'unpickle_map')
sage: pg_HeckeModuleMorphism_matrix = unpickle_global('sage.modular.hecke.morphism', 'HeckeModuleMorphism_matrix')
sage: pg_Hom = unpickle_global('sage.categories.homset', 'Hom')
sage: pg_dynamic_class = unpickle_global('sage.structure.dynamic_class', 'dynamic_class')
sage: pg_ModularSymbolsAmbient_wt2_g0 = unpickle_global('sage.modular.modsym.ambient', 'ModularSymbolsAmbient_wt2_g0')
sage: pg_getattr = unpickle_global('__builtin__', 'getattr')
sage: pg_unreduce = unpickle_global('sage.structure.unique_representation', 'unreduce')
sage: pg_HeckeModules = unpickle_global('sage.categories.hecke_modules', 'HeckeModules')
sage: pg_RationalField = unpickle_global('sage.rings.rational_field', 'RationalField')
sage: pg = unpickle_instantiate(pg_RationalField, ())
sage: si1 = pg_unreduce(pg_HeckeModules, (pg,), {})
sage: si2 =
unpickle_newobj(pg_dynamic_class('ModularSymbolsAmbient_wt2_g0_with_category',
(pg_ModularSymbolsAmbient_wt2_g0, pg_getattr(si1, 'parent_class')),
None, None, pgsage: _ModularSymbolsAmbient_wt2_g0), ())
sage: pg_unpickle = unpickle_global('sage.matrix.matrix0', 'unpickle')
sage: pg_Matrix_rational_dense = unpickle_global('sage.matrix.matrix_rational_dense', 'Matrix_rational_dense')
sage: pg_MatrixSpace = unpickle_global('sage.matrix.matrix_space', 'MatrixSpace')
sage: si3 = pg_unreduce(pg_MatrixSpace, (pg, 3r, 3r, False), {})
sage: si4 = pg_unpickle(pg_Matrix_rational_dense, si3, True, None, '8 0 0 0 8 0 0 0 8', 0r)
sage: pg__make_p1list = unpickle_global('sage.modular.modsym.p1list', '_make_p1list')
sage: pg_Gamma0_constructor = unpickle_global('sage.modular.arithgroup.congroup_gamma0', 'Gamma0_constructor')
sage: pg_make_integer = unpickle_global('sage.rings.integer', 'make_integer')
sage: si5 = pg_make_integer('6')
sage: pg_generic_factory_unpickle = unpickle_global('sage.structure.factory', 'generic_factory_unpickle')
sage: pg_lookup_global = unpickle_global('sage.structure.factory', 'lookup_global')
sage: si6 = (6r, 2r, 'rc1')
sage: pg_ManinSymbol = unpickle_global('sage.modular.modsym.manin_symbols', 'ManinSymbol')
sage: si7 = unpickle_newobj(pg_ManinSymbol, ())
sage: si8 = (0r, 0r, 1r)
sage: pg_ManinSymbolList_gamma0 = unpickle_global('sage.modular.modsym.manin_symbols', 'ManinSymbolList_gamma0')
sage: si9 = unpickle_newobj(pg_ManinSymbolList_gamma0, ())
sage: si10 = unpickle_newobj(pg_ManinSymbol, ())
sage: si11 = (0r, 1r, 0r)
sage: unpickle_build(si10, {'_ManinSymbol__t':si11, '_ManinSymbol__parent':si9})
sage: si12 = unpickle_newobj(pg_ManinSymbol, ())
sage: si13 = (0r, 1r, 1r)
sage: unpickle_build(si12, {'_ManinSymbol__t':si13, '_ManinSymbol__parent':si9})
sage: si14 = unpickle_newobj(pg_ManinSymbol, ())
sage: si15 = (0r, 1r, 2r)
sage: unpickle_build(si14, {'_ManinSymbol__t':si15, '_ManinSymbol__parent':si9})
sage: si16 = unpickle_newobj(pg_ManinSymbol, ())
sage: si17 = (0r, 1r, 3r)
sage: unpickle_build(si16, {'_ManinSymbol__t':si17, '_ManinSymbol__parent':si9})
sage: si18 = unpickle_newobj(pg_ManinSymbol, ())
sage: si19 = (0r, 1r, 4r)
sage: unpickle_build(si18, {'_ManinSymbol__t':si19, '_ManinSymbol__parent':si9})
sage: si20 = unpickle_newobj(pg_ManinSymbol, ())
sage: si21 = (0r, 1r, 5r)
sage: unpickle_build(si20, {'_ManinSymbol__t':si21, '_ManinSymbol__parent':si9})
sage: si22 = unpickle_newobj(pg_ManinSymbol, ())
sage: si23 = (0r, 2r, 1r)
sage: unpickle_build(si22, {'_ManinSymbol__t':si23, '_ManinSymbol__parent':si9})
sage: si24 = unpickle_newobj(pg_ManinSymbol, ())
sage: si25 = (0r, 2r, 3r)
sage: unpickle_build(si24, {'_ManinSymbol__t':si25, '_ManinSymbol__parent':si9})
sage: si26 = unpickle_newobj(pg_ManinSymbol, ())
sage: si27 = (0r, 2r, 5r)
sage: unpickle_build(si26, {'_ManinSymbol__t':si27, '_ManinSymbol__parent':si9})
sage: si28 = unpickle_newobj(pg_ManinSymbol, ())
sage: si29 = (0r, 3r, 1r)
sage: unpickle_build(si28, {'_ManinSymbol__t':si29, '_ManinSymbol__parent':si9})
sage: si30 = unpickle_newobj(pg_ManinSymbol, ())
sage: si31 = (0r, 3r, 2r)
sage: unpickle_build(si30, {'_ManinSymbol__t':si31, '_ManinSymbol__parent':si9})
sage: unpickle_build(si9, {'_ManinSymbolList__manin_symbol_list':[si7,
si10, si12, si14, si16, si18, si20, si22, si24, si26, si28, si30],
'_list':[si8, si11, si13, si15, si1sage: 7, si19, si21, si23, si25,
si27, si29, si31], '_index':{si13:2r, si27:9r, si15:3r, si8:0r,
si17:4r, si25:8r, si23:7r, si31:11r, si19sage: :5r, si11:1r, si29:10r,
si21:6r}, '_ManinSymbolList_group__syms':pg__make_p1list(6r),
'_ManinSymbolList_group__level':si5, '_wsage: eight':2r})
sage: unpickle_build(si7, {'_ManinSymbol__t':si8, '_ManinSymbol__parent':si9})
sage: pg_Matrix_rational_sparse = unpickle_global('sage.matrix.matrix_rational_sparse', 'Matrix_rational_sparse')
sage: pg_make_rational = unpickle_global('sage.rings.rational', 'make_rational')
sage: si32 = {(3r, 2r):pg_make_rational('1'), (0r,
0r):pg_make_rational('-1'), (8r, 2r):pg_make_rational('-1'), (7r,
1r):pg_make_rational('1'), (9r, 2r):pg_make_rational('-1sage: '), (5r,
2r):pg_make_rational('1'), (11r, 2r):pg_make_rational('1'), (3r,
1r):pg_make_rational('-1'), (10r, 1r)sage: :pg_make_rational('1'),
(5r, 1r):pg_make_rational('-1'), (1r, 0r):pg_make_rational('1'), (4r,
1r):pg_make_rational('-1'), (7r, 2r)sage: :pg_make_rational('-1'), (9r, 1r):pg_make_rational('1')}
sage: pg_DirichletCharacter = unpickle_global('sage.modular.dirichlet', 'DirichletCharacter')
sage: si33 = unpickle_newobj(pg_DirichletCharacter, ())
sage: si34 = pg_make_integer('6')
sage: unpickle_build(si33,
(pg_generic_factory_unpickle(pg_lookup_global('DirichletGroup'), si6,
(pg, si34, pg_make_rational('-1'), pg_make_integer('2')), {}),
{'_DirichletCsage: haracter__modulus':si34, '_DirichletCharacter__values_on_gens':(pg_make_rational('1'),)}))
sage: pg_HeckeAlgebra_full = unpickle_global('sage.modular.hecke.algebra', 'HeckeAlgebra_full')
sage: pg_CommutativeAlgebras = unpickle_global('sage.categories.commutative_algebras', 'CommutativeAlgebras')
sage: si35 =
unpickle_newobj(pg_dynamic_class('HeckeAlgebra_full_with_category',
(pg_HeckeAlgebra_full, pg_getattr(pg_unreduce(pg_CommutativeAlgebras,
(pg,), {}), 'parent_clsage: ass')), None, None, pg_HeckeAlgebra_full), ())
sage: pg_HeckeOperator = unpickle_global('sage.modular.modsym.hecke_operator', 'HeckeOperator')
sage: si36 = unpickle_newobj(pg_HeckeOperator, ())
sage: pg_HeckeAlgebraElement_matrix = unpickle_global('sage.modular.hecke.hecke_operator', 'HeckeAlgebraElement_matrix')
sage: si37 = unpickle_newobj(pg_HeckeAlgebraElement_matrix, ())
sage: unpickle_build(si37, (si35,
{'_HeckeAlgebraElement_matrix__matrix':si4,
'_HeckeAlgebraElement__hecke_module_morphism':pg_unpickle_map(pg_HeckeModuleMorphism_matrix,
pgsage: _Hom(si2, si2, si1), {'_matrix':si4, '_HeckeModuleMorphism_matrix__name':''}, {'_codomain':si2, '_domain':si2, '_repr_type_str':None})}))
sage: unpickle_build(si36, (si35, {'_HeckeOperator__matrix_form':si37, '_HeckeOperator__matrix':si4, '_HeckeOperator__n':7r}))
sage: unpickle_build(si35, {'_latex_names':None, '_list':None,
'_HeckeAlgebra_base__matrix_space_cache':si3, '_names':None,
'_gens_dict':None, '_HeckeAlgebra_base__M':si2, 'sage: _base':pg, '_gens':None, '_HeckeAlgebra_base__hecke_operator':{7r:si36}})
sage: unpickle_build(si2, {'_latex_names':None, '_ModularSymbolsAmbient__rank':3r, '_hecke_matrices':{(7r, None):si4, 7r:si4}, '_ModularSymbolsAmbient__p1list':pg__make_p1list(6r), '_ModularSymbolsSpace__group':pg_Gamma0_constructor(si5), '_AmbientHeckeModule__free_module':pg_generic_factory_unpickle(pg_lookup_global('FreeModule'), si6, (pg, 3r, False, None), {}), '_ModularSymbolsSpace__sign':0r, '_AmbientHeckeModule__rank':pg_make_integer('3'), '_HeckeModule_free_module__weight':2r, '_list':None, '_manin_generators':[si7, si10, si12, si14, si16, si18, si20, si22, si24, si26, si28, si30], '_gens_dict':None, '_generator_orders':None, '_manin_gens_to_basis':pg_unpickle(pg_Matrix_rational_sparse, pg_unreduce(pg_MatrixSpace, (pg, 12r, 3r, True), {}), False, {'dict':si32}, si32, -1r), '_ModularSymbolsSpace__character':si33, '_diamond_matrices':{}, '_names':None, '_HeckeModule_generic__level':si5, '_manin_basis':[1r, 10r, 11r], '_base':pg, '_gens':None, '_HeckeModule_generic__hecke_algebra':si35, '_mod2term':[(1r, pg_make_rational('-1')), (1r, pg_make_rational('1')), (6r, pg_make_rational('-1')), (9r, pg_make_rational('-1')), (10r, pg_make_rational('-1')), (7r, pg_make_rational('-1')), (6r, pg_make_rational('1')), (7r, pg_make_rational('1')), (11r, pg_make_rational('-1')), (9r, pg_make_rational('1')), (10r, pg_make_rational('1')), (11r, pg_make_rational('1'))], '_ModularSymbolsAmbient_wtk_g0__manin_symbols':si9})
sage: pg_unpickle_map(pg_HeckeModuleMorphism_matrix, pg_Hom(si2, si2, si1), {'_matrix':pg_unpickle(pg_Matrix_rational_dense, si3, False, None, '0 1 2 3 4 5 6 7 8', 0r), '_HeckeModuleMorphism_matrix__name':'spam', '_default_filename':'broken_pickle.sobj'}, {'_codomain':si2, '_domain':si2, '_repr_type_str':None})

Cheers,
Nicolas

[simon-king-jena]

comment:43

I think the update of the pickle jar is not sooooo urgent. #11720 asks for a regular update of the pickle jar, though. One could argue that we did care of the potential problem, and (as you said before) the probability of finding important pickles that would now be broken is relatively slim.

So, I'd say it is a positive review.

[vbraun]

Changed branch from u/SimonKing/ticket/16275 to e1e916c

[jdemeyer]

comment:46

Could you please justify your use of except BaseException instead of except Exception. This looks like a bug to me, but I might be missing something.

[jdemeyer]

Changed commit from e1e916c to none

[nthiery]

comment:47

Replying to @jdemeyer:

Could you please justify your use of except BaseException instead of except Exception. This looks like a bug to me, but I might be missing something.

I have no specific insight there: the previous code was using BaseException at this point (or rather the analogue thereof) and I just carried it over. Indeed Exception seems to make more sense; I might be missing something too though.

[simon-king-jena]

comment:48

In one of the commits, I see

except BaseException:
    # An error should not happen, this here is just to be on
    # the safe side.
    category_mismatch = True

This pretty much explains it. I have never seen such error. However, if for some silly reason the test "O in category" does not return False but results in any error (but not KeyboardInterrupt), then I want to count it as a mismatch.

[simon-king-jena]

comment:49

If I am not mistaken, KeyboardInterrupt is not a BaseException, and indeed I don't want that a KeyboardInterrupt is caught at that point.

[jdemeyer]

comment:50

Replying to @simon-king-jena:

If I am not mistaken, KeyboardInterrupt is not a BaseException

You are mistaken.

BaseException is the common base for every kind of exception, including the ones which you almost never want to catch.

[simon-king-jena]

comment:51

Oops, it is a BaseException. Is Exception less broad? Then I am all for changing it.

[jdemeyer]

comment:52

The bottom line is: if you want to catch every "normal" exception, use except Exception:.

The only use cases for except BaseException: are when you re-raise the exception (after doing some clean-up) and for very particular low-level stuff.

[simon-king-jena]

comment:53

Replying to @jdemeyer:

The bottom line is: if you want to catch every "normal" exception, use except Exception:.

The only use cases for except BaseException: are when you re-raise the exception (after doing some clean-up) and for very particular low-level stuff.

OK. I agree it ought to be changed, and if it can wait till tomorrow then I'll change it myself.

[jdemeyer]

comment:54

I will fix it on #17076.