-
-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make parent/element classes independent of base rings #11935
Comments
comment:1
Concerning uniqueness of the parent class: In at least one case (namely
The idea is that the parent and element classes should only depend on the super categories, but otherwise should be independent from the base ring. Working at #11900, I found that this would drastically improve the performance of some elliptic curve computation. |
comment:2
A problem may be seen in pickling. Before explaining the problem, let me remark that I don't see a big concern for "pickling a parent class": What we actually want to pickle is a parent, not just a naked class. The serialisation data of a polynomial ring, for example, will comprise the base ring, the generator names and the term order, but certainly the class of the polynomial ring will not be part of the pickle. However, if we do want to serialise a naked parent or element class, we have the following problems: Currently, I see three approaches to get rid of it.
I think 1. is not gonna happen. It would break a lot of code, I suppose. I had tested 2. and 3. while working on #11900. Both would work, but there are different conerns concerning long-term stability.
I suggest that I create patches for both 2. and 3., and then people can tell what they think about it. The method resolution will then be taken care of by another patch. |
comment:3
Replying to @simon-king-jena:
True: all parents ought to be pickled "by construction" rather than by A good thing to do at this point would be to search through the sage Among the three propositions, I like 3 best. I have trouble evaluating Thanks Simon for investigating this! |
This comment has been minimized.
This comment has been minimized.
comment:4
Replying to @simon-king-jena:
I just argued myself into splitting the ticket: This here will be for the base ring independent parent/element classes, and another one will be for method resolution order. |
comment:5
Replying to @nthiery:
Old pickles will not break, I believe. Let 1. P3 and PQ have been created and pickled with an old version of Sage In the old version of Sage, Conclusion: An old pickle will not break, with either approach 2. or 3. The worst what could happen is 2. P3 and PQ are created and pickled according to approach 2. from above Of course, Conclusion: A new pickle of P3 and PQ can be unpickled after a change in the category graph, but a change in the category graph will destroy the defining property 3. P3 and PQ are created and pickled according to approach 3. from above Of course, A problem would arise if, in a distant future, the super categories of Conclusion: A new pickle of P3 and PQ can be unpickled after a change in the category graph. Most changes in the category graph will preserve the defining property It seems to me that approach 3. is less fragile than 2. But I believe that in applications (hence, for pickling parents) both should be fine. So, I'll prepare patches for both approaches. |
Use default pickling for parent/element classes, making them base ring independent. |
comment:6
Attachment: trac11935_use_default_pickling.patch.gz The patch that I just attached implements approach 2., hence, it uses the default pickling of dynamic classes. By consequence, the parent class of a category C will only depend on Note the effect on the computation time for elliptic curves. With sage-4.7.2.alpha3 plus #11900, we have
but with the new patch on top of it, we have
|
Author: Simon King |
Attachment: trac11935_weak_pickling_by_construction.patch.gz Use a weak form of "pickling by construction" for parent and element classes of categories |
comment:7
The second patch is posted as well. It implements approach 3. Hence, the parent_class lazy attribute works around the cache of dynamic classes (by not providing unpickling information when the class is created), inserting the unpickling information only when the class has not been found in cache. By consequence, when first creating Similar to the first patch, it considerably speeds up the elliptic curve computations:
Apply only one of the two patches, at your choice! |
comment:8
Replying to @simon-king-jena:
This is my belief too! Sorry if I have been unclear, but that was not the point of my suggestion. What I wanted to know whether currently most parents and elements were pickled by construction or by "class + data". If they already are pickled by construction, then how C.parent_class and C.element_class are pickled is mostly irrelevant, now and in the future, since it is seldom used. Thanks in any cases for you detailed analysis!
+1 Let's see if someone else has some preference between the two implementations. Cheers, |
comment:9
Replying to @nthiery:
I see. If I am not mistaken, if it is pickled by "class+data", then copy_reg._reconstructor is called at unpickling. Perhaps it is possible to modify _reconstructor, so that it writes data to some log file. In that way, we could find out how often it is actually used. |
comment:10
Replying to @simon-king-jena:
Yup. Or run explain_pickle on all pickles, and grep for element_class / parent_class. |
comment:11
Replying to @nthiery:
I don't know about explain_pickle. Where can I find it? I am now running sage -testall -long with the _reconstructor log. So far, only ZODB.fsIndex.fsIndex and matplotlib.font_manager.FontEntry use it, but we will see if there's more. |
comment:12
sage -testall -long succeeded (with the second patch applied, but it would probably work with thee first one as well), and copy_reg._reconstructor was only used on Hence, it indicates that "pickling by class and data" does not occur for parents. But, to be on the safe side, one should inspect the pickle jar using explain_pickle. |
comment:14
|
This comment has been minimized.
This comment has been minimized.
comment:15
I decided to make this ticket depend on #11943, for two reasons: Firstly, it is rather clear that #11943 is a good idea, while I am less sure here (it is good for speed, but may under very particular circumstances break new pickles). Secondly, #11943 seems less invasive than the patch here. I think that the "potentially breaking new pickles in a distant future" aspect is less urgent with the "weak pickling by construction" approach. Hence, I have only updated the second patch, the first can now be disregarded. Apply trac11935_weak_pickling_by_construction_rel11943.patch |
This comment has been minimized.
This comment has been minimized.
comment:112
But who can review it? I guess as the author, I am only entitled to tell whether I am fine with the changes made by Nicolas and Travis. So, what else can I do? #12895, or is there something more urgent? |
comment:113
For the record: I think Travis' patch is fine. |
comment:114
I already reviewed your things. So if you are happy with my changes, then it's good to go! Yes, #12895 is next on the list! Thanks! |
comment:115
I think Nicolas' changes look fine, too. But let's wait for the tests to finish. |
comment:116
All tests had passed. However I've uploaded a tweaked review patch which changes the doctest line continuations to the new format. Apply: trac11935_weak_pickling_by_construction-nt.patch trac_11935-weak_pickling_by_construction-review-ts.patch |
comment:117
Ok. +1 on Travis's new patch. |
comment:118
Travis' changes look good. So, a final make ptestlong, and then we'll hopefully get it over with. |
comment:119
The patchbot finds that all tests pass. On my laptop, tests aren't finished yet, but nothing has failed so far. The startup.modules script complains that now the inspect module is loaded. This is a bit surprising to me, because I thought that the inspect module is loaded during startup anyway (it is frequently used). But I think there is not much need to worry---and the startup-time plugin does not complain. The doctest continuation plugin complains about the first patch, but I just verified that the second patch fixes it. And given all the comments above, I hope I am no entitled to change the status to "positive_review"---complain if you disagree. |
comment:120
Yippee :-) |
comment:122
Hi Jeroen, Replying to @jdemeyer:
Ah shoot, it would have been helpful to have a clean base including those patches for the work on the subsequent category patches. I assume you are in the process of cutting a rc0? Do you have some approximate schedule for 5.11? Thanks! |
comment:123
Replying to @nthiery:
Well, given that these kind of patches are notorious for breaking things, I don't want to merge this in an rc. This patch is scheduled for sage-5.11.beta1 (with #12876 in beta0).
What do you mean? |
comment:124
Replying to @jdemeyer:
Fair enough :-)
Good point; we only need to have them in a beta, not necessarily for a final release.
Roughly speaking when do you foresee 5.11.beta1 to be out? Thanks! |
Merged: sage-5.11.beta1 |
Changed reviewer from Nicolas Thiery, Travis Scrimshaw to Nicolas M. Thiéry, Travis Scrimshaw |
At #11900 and sage-combinat-devel, as well as in some comments in sage/categories/category.py, the idea was discussed to make, for example,
Algebras(GF(3)).parent_class==Algebras(GF(5)).parent_class
- hence, make the parent/element classes as independent from the base of a category as possible.This is implemented in this patch by introducing an abstract class
CategoryWithParameters which uses pickling by "weak construction" for
its element and parent classes. Now:
parent/element class of its super categories.
parent/element class depend only on the category of the base.
of the left and right bases.
In the process, this patch also:
create parent and element classes (and later on morphism classes)
and remove its super class, and discards unused code there.
Apply
Depends on #9138
Depends on #11900
Depends on #11943
Depends on #12875
Depends on #12876
Depends on #12877
CC: @jdemeyer @sagetrac-sage-combinat
Component: categories
Keywords: parent class, element class
Author: Simon King
Reviewer: Nicolas M. Thiéry, Travis Scrimshaw
Merged: sage-5.11.beta1
Issue created by migration from https://trac.sagemath.org/ticket/11935
The text was updated successfully, but these errors were encountered: