Opened 6 months ago

Closed 6 months ago

Last modified 4 weeks ago

#16275 closed enhancement (fixed)

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

Reported by: nthiery Owned by:
Priority: major Milestone: sage-6.3
Component: categories Keywords: homset, pickling
Cc: SimonKing, tscrim Merged in:
Authors: Nicolas M. Thiéry, Simon King Reviewers: Simon King, Nicolas M. Thiéry
Report Upstream: N/A Work issues:
Branch: e1e916c (Commits) Commit:
Dependencies: Stopgaps:

Description

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 #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).

Attachments (1)

broken_pickle.sobj (1.8 KB) - added by SimonKing 6 months ago.
A failing pickle

Download all attachments as: .zip

Change History (55)

comment:1 Changed 6 months ago by nthiery

  • Branch set to u/nthiery/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic

comment:2 Changed 6 months ago by git

  • Commit set to 1ff993bb8e1af3eaae79255a4b97cc4b5afe39bf

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

1ff993b#16275: trivial commit to use format

comment:3 follow-up: Changed 6 months ago by 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?

comment:4 Changed 6 months ago by nthiery

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

comment:5 Changed 6 months ago by git

  • Commit changed from 1ff993bb8e1af3eaae79255a4b97cc4b5afe39bf to 0074f0259a502a2e7fa6d93f1814c418b8dfb1c1

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

comment:6 Changed 6 months ago by git

  • Commit changed from 0074f0259a502a2e7fa6d93f1814c418b8dfb1c1 to ae3ca8080a8412891722695d1e24e1dfe374d787

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

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

comment:7 Changed 6 months ago by nthiery

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

comment:8 in reply to: ↑ description Changed 6 months ago by SimonKing

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__.

comment:9 in reply to: ↑ 3 ; follow-up: Changed 6 months ago by SimonKing

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.

comment:10 follow-up: Changed 6 months ago by SimonKing

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.

Changed 6 months ago by SimonKing

A failing pickle

comment:11 Changed 6 months ago by SimonKing

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?

comment:12 Changed 6 months ago by SimonKing

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.

comment:13 Changed 6 months ago by SimonKing

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.

comment:14 Changed 6 months ago by SimonKing

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??

comment:15 in reply to: ↑ 10 Changed 6 months ago by nthiery

Replying to SimonKing:

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.

comment:16 in reply to: ↑ 9 ; follow-up: Changed 6 months ago by nthiery

Replying to SimonKing:

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

comment:17 Changed 6 months ago by nthiery

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

comment:18 in reply to: ↑ 16 ; follow-up: Changed 6 months ago by SimonKing

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.

comment:19 in reply to: ↑ 18 Changed 6 months ago by SimonKing

Replying to SimonKing:

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?

comment:20 Changed 6 months ago by git

  • Commit changed from ae3ca8080a8412891722695d1e24e1dfe374d787 to 507ee00f4f1f81680a52cd798d2fd5a03fb4172d

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

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

comment:21 Changed 6 months ago by nthiery

I allowed myself to do the fixes in homspace since I wanted to merge the branch in the current state in #15801.
Hope this will not conflict ...

Last edited 6 months ago by nthiery (previous) (diff)

comment:22 Changed 6 months ago by SimonKing

ARRGH!

I have just pushed my stuff as well!

So, where has it gone?


New commits:

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

New commits:

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

comment:23 Changed 6 months ago by SimonKing

OK, since you have taken care of the two other points, I will now try to make a new commit including the additional check. PLEASE DO NOT PUSH IN THE NEXT FIVE MINUTES!

comment:24 Changed 6 months ago by SimonKing

I am puzzled. I have pulled from your branch. Then I have added a commit, and tried git trac push and git push. The former failed without telling me why. The latter failed, and told me

To git@trac.sagemath.org:sage.git
 ! [rejected]        t/16275/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic -> u/SimonKing/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic (non-fast-forward)
error: Fehler beim Versenden einiger Referenzen nach 'git@trac.sagemath.org:sage.git'
Hinweis: Aktualisierungen wurden zurückgewiesen, weil die Spitze eines versendeten
Hinweis: Zweiges hinter seinem externen Gegenstück zurückgefallen ist. Checke diesen
Hinweis: Zweig aus und führe die externen Änderungen zusammen (z.B. 'git pull')
Hinweis: bevor du erneut versendest.
Hinweis: Siehe auch die Sektion 'Note about fast-forwards' in 'git push --help'
Hinweis: für weitere Details.

In other words, it claims that my branch tip is BEHIND the tip of the current branch here. But this is not the case! I did pull before committing, and here is the log:

* 76bdaa9 - (HEAD, t/16275/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic) Make it so that old homset pickles remain unpicklable (vor 4 Minuten) <Simon King>
* 507ee00 - Trac 16275: fixes for the previous commit; btw its title should have read: removed sage.modular.abvar.homspace.Homspace.__reduce__ (vor 12 Minuten) <Nicolas M. Thiéry>
* ae3ca80 - This is handled as well by the generic of Homset.__reduce__ method, and the latter takes care of setting check=False as needed. (vor 4 Stunden) <Nicolas M. Thiéry>
* c1e91f3 - #16275: update ticket number in the comments (vor 4 Stunden) <Nicolas M. Thiéry>

comment:25 Changed 6 months ago by SimonKing

  • Branch changed from u/nthiery/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic to u/SimonKing/ticket/16275
  • Created changed from 05/01/14 15:40:52 to 05/01/14 15:40:52
  • Modified changed from 05/02/14 07:49:06 to 05/02/14 07:49:06

comment:26 Changed 6 months ago by SimonKing

  • Commit changed from 507ee00f4f1f81680a52cd798d2fd5a03fb4172d to 76bdaa90610e20d51456db092883ff7c61e5d6c2
  • Reviewers set to Simon King
  • Status changed from new to needs_review

It worked when I pushed with sage -dev push --ticket=16275, rather than git push or git trac push. Let's see if all tests pass. If they do and if you accept my suggestions, then this can be positive review.


New commits:

76bdaa9Make it so that old homset pickles remain unpicklable

comment:27 follow-up: Changed 6 months ago by nthiery

Sorry for the branch mess ...

It's a bit convoluted, but I don't have a good alternative to support those pickles, so that's ok.

Maybe we could save on the catching of exceptions upon building the error message? In principle,
there should be no error message upon unpickling; and otherwise, the input is supposed to be properly initialized, and in particular should have a working repr, shouldn't it?

Do you mind adding a few comments in the code, say a pointer to the discussion here + mention of the pickle added in the pickle jar? (or I can do it).

In any cases, thanks much!

Cheers,

Nicolas

comment:28 in reply to: ↑ 27 Changed 6 months ago by SimonKing

Replying to nthiery:

Maybe we could save on the catching of exceptions upon building the error message? In principle,
there should be no error message upon unpickling; and otherwise, the input is supposed to be properly initialized, and in particular should have a working repr, shouldn't it?

Sounds right. So, perhaps the additional care for the error message can be removed.

Do you mind adding a few comments in the code, say a pointer to the discussion here + mention of the pickle added in the pickle jar? (or I can do it).

OK, I will do. But it is not put in the pickle jar yet, since I don't know how an update of the pickle jar will be posted on trac. In this sense, it still needs work.

comment:29 Changed 6 months ago by SimonKing

Oops, I see that I need to change the expected output of another test, as I change the TypeError into a ValueError (which I think is appropriate).

comment:30 Changed 6 months ago by git

  • Commit changed from 76bdaa90610e20d51456db092883ff7c61e5d6c2 to ecc4616852de61a3f1fa62bdf9dc76a61d94aaba

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

ecc4616Fix one test and simplify the logic of unpickling old homset pickles

comment:31 Changed 6 months ago by SimonKing

Fixed. Hopefully. I'd wait for the tests to finish before giving a positive review...

comment:32 Changed 6 months ago by SimonKing

Alas, not everything that is using Hom has _is_category_initialized. SimplicialComplex does not inherit from CategoryObject, which is a shame.

comment:33 follow-up: Changed 6 months ago by nthiery

"belonging -> belongs" in the code comment. Other than that your commit looks good. Thanks!

What about including the pickle directly as a string in the docttest (or possibly in the code)? I think we have already done this on some other occasion, and this sounds appropriate to point out: we want this specific pickle to work, rather than this is tested by some pickle out there in the pickle jar; go search for it if you want to reproduce the potential problem.

Ah, yes, there remains to add a little ref to the ticket in the code comments. Do you want me to do it?

comment:34 in reply to: ↑ 33 ; follow-up: Changed 6 months ago by SimonKing

Replying to nthiery:

"belonging -> belongs" in the code comment. Other than that your commit looks good. Thanks!

OK, can do. But before pushing it:

What about including the pickle directly as a string in the docttest (or possibly in the code)? I think we have already done this on some other occasion, and this sounds appropriate to point out: we want this specific pickle to work, rather than this is tested by some pickle out there in the pickle jar; go search for it if you want to reproduce the potential problem.

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. So, I still think the pickle jar is the right place, whereas a cryptic to-be-unpickled string somewhere in the docs seems odd to me.

Ah, yes, there remains to add a little ref to the ticket in the code comments.

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

Do you want me to do it?

I'll do it. I have to take care of the simplicial complexes anyway.

But how to proceed? Shall I make simplicial complexes inherit from CategoryObject right here, or shall I just silence the underlying error and postpone the fix for simplicial complexes?

comment:35 Changed 6 months ago by SimonKing

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.

comment:36 Changed 6 months ago by SimonKing

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?

comment:37 Changed 6 months ago by git

  • Commit changed from ecc4616852de61a3f1fa62bdf9dc76a61d94aaba to e1e916cde347deac1df9079f7ec856184e3862bf

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

e1e916cSimplicial complexes are now CategoryObject. Switch TypeError into ValueError

comment:38 Changed 6 months ago by SimonKing

  • Authors changed from Nicolas M. Thiéry to Nicolas M. Thiéry, Simon King

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.

comment:39 in reply to: ↑ 34 Changed 6 months ago by nthiery

Replying to SimonKing:

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,

Last edited 6 months ago by nthiery (previous) (diff)

comment:40 Changed 6 months ago by nthiery

  • Reviewers changed from Simon King to Simon King, Nicolas M. Thiéry

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.

comment:41 Changed 6 months ago by SimonKing

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.

comment:42 Changed 6 months ago by nthiery

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

comment:43 Changed 6 months ago by SimonKing

  • Status changed from needs_review to positive_review

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.

comment:44 Changed 6 months ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:45 Changed 6 months ago by vbraun

  • Branch changed from u/SimonKing/ticket/16275 to e1e916cde347deac1df9079f7ec856184e3862bf
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:46 follow-up: Changed 4 weeks ago by jdemeyer

  • Commit e1e916cde347deac1df9079f7ec856184e3862bf deleted

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.

comment:47 in reply to: ↑ 46 Changed 4 weeks ago by nthiery

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.

comment:48 Changed 4 weeks ago by SimonKing

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.

comment:49 follow-up: Changed 4 weeks ago by SimonKing

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

comment:50 in reply to: ↑ 49 Changed 4 weeks ago by jdemeyer

Replying to SimonKing:

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.

comment:51 Changed 4 weeks ago by SimonKing

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

comment:52 follow-up: Changed 4 weeks ago by 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.

comment:53 in reply to: ↑ 52 Changed 4 weeks ago by SimonKing

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.

comment:54 Changed 4 weeks ago by jdemeyer

I will fix it on #17076.

Note: See TracTickets for help on using tickets.