#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)
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
comment:3 follow-up: ↓ 9 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
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: ↓ 16 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: ↓ 15 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.
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: ↓ 18 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 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 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: ↓ 19 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:
507ee00 | Trac 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 ...
comment:22 Changed 6 months ago by SimonKing
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 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:
76bdaa9 | Make it so that old homset pickles remain unpicklable |
comment:27 follow-up: ↓ 28 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:
ecc4616 | Fix 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: ↓ 34 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: ↓ 39 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:
e1e916c | Simplicial complexes are now CategoryObject. Switch TypeError into ValueError |
comment:38 Changed 6 months ago by SimonKing
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,
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: ↓ 47 Changed 3 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 3 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 3 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: ↓ 50 Changed 3 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 3 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 3 weeks ago by SimonKing
Oops, it *is* a BaseException. Is Exception less broad? Then I am all for changing it.
comment:52 follow-up: ↓ 53 Changed 3 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 3 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 3 weeks ago by jdemeyer
I will fix it on #17076.
Branch pushed to git repo; I updated commit sha1. New commits: