#16275 closed enhancement (fixed)
Hom: introduce a check argument to simplify the unpickling detection logic
Reported by:  nthiery  Owned by:  

Priority:  major  Milestone:  sage6.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 aQQ
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)
Change History (55)
comment:1 Changed 3 years ago by
 Branch set to u/nthiery/hom__introduce_a_check_argument_to_simplify_the_unpickling_detection_logic
comment:2 Changed 3 years ago by
 Commit set to 1ff993bb8e1af3eaae79255a4b97cc4b5afe39bf
comment:3 followup: ↓ 9 Changed 3 years ago by
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 3 years ago by
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^21,y^21]) 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 3 years ago by
 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 3 years ago by
 Commit changed from 0074f0259a502a2e7fa6d93f1814c418b8dfb1c1 to ae3ca8080a8412891722695d1e24e1dfe374d787
comment:7 Changed 3 years ago by
With the latest commit, all tests pass. Running the long tests now.
comment:8 in reply to: ↑ description Changed 3 years ago by
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 "tryexcept" 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 ; followup: ↓ 16 Changed 3 years ago by
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 tobeunpickled object belongs to the category of the tobeunpickled 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 followup: ↓ 15 Changed 3 years ago by
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.
comment:11 Changed 3 years ago by
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 3 years ago by
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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/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/sitepackages/sage/categories/modules.pyc in __classcall_private__(cls, base_ring, dispatch) 106 if base_ring in _Fields: 107 return VectorSpaces(base_ring, check=False) > 108 result = super(Modules, cls).__classcall__(cls, base_ring) 109 result._reduction[2]['dispatch'] = False 110 return result /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/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/sitepackages/sage/misc/cachefunc.so in sage.misc.cachefunc.WeakCachedFunction.__call__ (sage/misc/cachefunc.c:6486)() /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/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/sitepackages/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/sitepackages/sage/categories/category_types.pyc in __init__(self, base, name) 324 def __init__(self, base, name=None): 325 from sage.categories.rings import Rings > 326 assert base in Rings(), "base must be a ring" 327 Category_over_base.__init__(self, base, name) 328 <type 'str'>: (<type 'exceptions.AttributeError'>, AttributeError('ModularSymbolsAmbient_wt2_g0_with_category' object has no attribute '_HeckeModule_generic__level',))
So, 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 3 years ago by
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 tryexcept around the creation of a string representation.
comment:14 Changed 3 years ago by
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) <ipythoninput1540ff1d7475d> in <module>() > 1 t = load("/home/king/Downloads/broken_pickle.sobj") /home/king/Sage/git/sage/local/lib/python2.7/sitepackages/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/sitepackages/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/sitepackages/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 3 years ago by
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 ; followup: ↓ 18 Changed 3 years ago by
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 homsetHence, 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 3 years ago by
For the record, up to the trivial failing test noted in 10, all long tests passed.
comment:18 in reply to: ↑ 16 ; followup: ↓ 19 Changed 3 years ago by
Replying to nthiery:
One thing:
domain not in category
will possibly calldomain.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 3 years ago by
Replying to SimonKing:
Replying to nthiery:
One thing:
domain not in category
will possibly calldomain.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 3 years ago by
 Commit changed from ae3ca8080a8412891722695d1e24e1dfe374d787 to 507ee00f4f1f81680a52cd798d2fd5a03fb4172d
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__

comment:21 Changed 3 years ago by
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 ...
comment:22 Changed 3 years ago by
ARRGH!
I have just pushed my stuff as well!
So, where has it gone?
New commits:
507ee00  Trac 16275: fixes for the previous commit; btw its title should have read: removed sage.modular.abvar.homspace.Homspace.__reduce__

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

comment:23 Changed 3 years ago by
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 3 years ago by
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 (nonfastforward) 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 fastforwards' 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 3 years ago by
 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 22:40:52 to 05/01/14 22:40:52
 Modified changed from 05/02/14 14:49:06 to 05/02/14 14:49:06
comment:26 Changed 3 years ago by
 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:
76bdaa9  Make it so that old homset pickles remain unpicklable

comment:27 followup: ↓ 28 Changed 3 years ago by
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 3 years ago by
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 3 years ago by
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 3 years ago by
 Commit changed from 76bdaa90610e20d51456db092883ff7c61e5d6c2 to ecc4616852de61a3f1fa62bdf9dc76a61d94aaba
Branch pushed to git repo; I updated commit sha1. New commits:
ecc4616  Fix one test and simplify the logic of unpickling old homset pickles

comment:31 Changed 3 years ago by
Fixed. Hopefully. I'd wait for the tests to finish before giving a positive review...
comment:32 Changed 3 years ago by
Alas, not everything that is using Hom has _is_category_initialized
. SimplicialComplex
does not inherit from CategoryObject
, which is a shame.
comment:33 followup: ↓ 34 Changed 3 years ago by
"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 ; followup: ↓ 39 Changed 3 years ago by
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 tobeunpickled 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 3 years ago by
sage.homology.simplicial_complex
is a mess, categorywise. 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 3 years ago by
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 3 years ago by
 Commit changed from ecc4616852de61a3f1fa62bdf9dc76a61d94aaba to e1e916cde347deac1df9079f7ec856184e3862bf
Branch pushed to git repo; I updated commit sha1. New commits:
e1e916c  Simplicial complexes are now CategoryObject. Switch TypeError into ValueError

comment:38 Changed 3 years ago by
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 3 years ago by
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,
comment:40 Changed 3 years ago by
 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 3 years ago by
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 sagedevel 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 3 years ago by
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 3 years ago by
 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 3 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:45 Changed 3 years ago by
 Branch changed from u/SimonKing/ticket/16275 to e1e916cde347deac1df9079f7ec856184e3862bf
 Resolution set to fixed
 Status changed from positive_review to closed
comment:46 followup: ↓ 47 Changed 3 years ago by
 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 3 years ago by
Replying to jdemeyer:
Could you please justify your use of
except BaseException
instead ofexcept 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 3 years ago by
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 followup: ↓ 50 Changed 3 years ago by
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 3 years ago by
Replying to SimonKing:
If I am not mistaken,
KeyboardInterrupt
is not aBaseException
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 3 years ago by
Oops, it *is* a BaseException
. Is Exception
less broad? Then I am all for changing it.
comment:52 followup: ↓ 53 Changed 3 years ago by
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 reraise the exception (after doing some cleanup) and for very particular lowlevel stuff.
comment:53 in reply to: ↑ 52 Changed 3 years ago by
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 reraise the exception (after doing some cleanup) and for very particular lowlevel 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 3 years ago by
I will fix it on #17076.
Branch pushed to git repo; I updated commit sha1. New commits:
#16275: trivial commit to use format