Make parent/element classes independent of base rings

[simon-king-jena]

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:

• For a join category, the parent/element class depend only on the parent/element class of its super categories.
• For a Category_over_base (e.g. Modules, Algebras, Schemes, ...), the parent/element class depend only on the category of the base.
• For a bimodule, the parent/element class depend only on the category of the left and right bases.

In the process, this patch also:

• Adds a method Category._make_named_class providing a unified way to create parent and element classes (and later on morphism classes)
• Extends the interface of dynamic_class to customize caching and pickling
• Rename the experimental class IdempotentSemigroups.ElementMethods and remove its super class, and discards unused code there.

Apply

• attachment: trac11935_weak_pickling_by_construction-nt.patch
• attachment: trac_11935-weak_pickling_by_construction-review-ts.patch

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

[simon-king-jena]

comment:38

I have implemented the new method _make_named_class (with "strong pickling by construction" for Category and "weak pickling by construction" for CategoryWithParameters). For me, all tests pass.

And, for the record:

sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 2.88 s, sys: 0.04 s, total: 2.92 s
Wall time: 3.06 s

Apply trac11935_weak_pickling_by_construction_rel11943.patch trac11935_named_class.patch

[simon-king-jena]

Changed work issues from introduce _make_named_class to none

[simon-king-jena]

Description changed:

--- 
+++ 
@@ -1,3 +1,6 @@
 At #11900 and [sage-combinat-devel](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/99c74827d704e677), 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.

-Apply [attachment: trac11935_weak_pickling_by_construction_rel11943.patch](https://github.com/sagemath/sage/files/ticket11935/trac11935_weak_pickling_by_construction_rel11943.patch.gz)
+__Apply__
+
+* [attachment: trac11935_weak_pickling_by_construction_rel11943.patch](https://github.com/sagemath/sage/files/ticket11935/trac11935_weak_pickling_by_construction_rel11943.patch.gz)
+* [attachment: trac11935_named_class.patch](https://github.com/sagemath/sage-prod/files/10653894/trac11935_named_class.patch.gz)

[simon-king-jena]

Work Issues: Rebase wrt the new version of #11900; ideas behind Category_singleton need to be modified if parent classes are shared

[simon-king-jena]

Changed work issues from Rebase wrt the new version of #11900; ideas behind Category_singleton need to be modified if parent classes are shared to none

[simon-king-jena]

comment:40

I had to rebase the first patch because of changes in #11943. It is now updated. The second patch did not need to change.

The timing is still good. I am now running the tests, but I think it can be needs review now.

Apply trac11935_weak_pickling_by_construction_rel11943.patch trac11935_named_class.patch

[simon-king-jena]

comment:41

FWIW: Tests pass.

[nthiery]

comment:42

For the record: all tests pass on 5.0 beta10 with the version of the patch on the Sage-Combinat queue (basically the two patches folded together).

[nthiery]

comment:43

I finished my review. From my point of view it's good to go!

Now I just realized that I had apparently already folded in some of my reviewers changes; sorry. Since the patch is not so long, I guess it's not so bad; I also folded in my latest little changes.

[nthiery]

Description changed:

--- 
+++ 
@@ -3,4 +3,4 @@
 __Apply__

 * [attachment: trac11935_weak_pickling_by_construction_rel11943.patch](https://github.com/sagemath/sage/files/ticket11935/trac11935_weak_pickling_by_construction_rel11943.patch.gz)
-* [attachment: trac11935_named_class.patch](https://github.com/sagemath/sage-prod/files/10653894/trac11935_named_class.patch.gz)
+

[nthiery]

comment:45

I guess we can delete the other intermediate patches once the review is finished.

Happy easter!

[nthiery]

comment:46

All tests seem to pass, except maybe for some fairly trivially failing tests in:

    sage -t  -force_lib "devel/sage/sage/categories/algebras.py"
    sage -t  -force_lib "devel/sage/sage/categories/modules_with_basis.py"
    sage -t  -force_lib "devel/sage/sage/categories/category.py"
    sage -t  -force_lib "devel/sage/sage/misc/preparser.py"

But that might be due to a couple other patches above in my queue (including #11943) when I ran the tests. I will investigate on Tuesday unless you beat me to it.

[nthiery]

comment:47

Replying to @nthiery:

All tests seem to pass, except maybe for some fairly trivially failing tests in ... But that might be due to a couple other patches above in my queue (including #11943) when I ran the tests. I will investigate on Tuesday unless you beat me to it.

Ok, only the failure in algebras.py was due to this patch (I had forgotten to update one of the subcategory hooks for the change NotImplemented -> Troolean). This is fixed with the updated patch I just posted.

Cheers, Nicolas

[simon-king-jena]

comment:48

The patch won't apply.

Wende trac11935_weak_pickling_by_construction_rel11943.patch an
patching file sage/categories/bimodules.py
Hunk #1 FAILED at 10
Hunk #3 succeeded at 118 with fuzz 1 (offset 0 lines).
1 out of 3 hunks FAILED -- saving rejects to file sage/categories/bimodules.py.rej
patching file sage/categories/category.py
Hunk #4 FAILED at 1083
Hunk #5 FAILED at 1114
Hunk #7 succeeded at 1632 with fuzz 2 (offset -157 lines).
Hunk #8 succeeded at 1686 with fuzz 2 (offset -179 lines).
2 out of 8 hunks FAILED -- saving rejects to file sage/categories/category.py.rej
patching file sage/categories/category_types.py
Hunk #1 FAILED at 14
1 out of 3 hunks FAILED -- saving rejects to file sage/categories/category_types.py.rej
Patch schlug fehl und Fortsetzung unmöglich (versuche -v)
Patch schlug fehl, Fehlerabschnitte noch im Arbeitsverzeichnis
Fehler beim Anwenden. Bitte beheben und auf trac11935_weak_pickling_by_construction_rel11943.patch aktualisieren
king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage$ hg qapplied 
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12313-mono_dict-combined.patch
trac11935_weak_pickling_by_construction_rel11943.patch

[simon-king-jena]

Work Issues: Rebase rel 5.0.beta13

[simon-king-jena]

comment:49

Arrgh, me stupid! I had #11943 in my queue, but after #11935. With

king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage$ hg qapplied 
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12313-mono_dict-combined.patch
trac11943_mro_for_all_super_categories_lazy_hook.patch
trac11943_mro_for_all_super_categories_lazy_hook-review-nt.patch
trac11935_weak_pickling_by_construction_rel11943.patch

everything is fine.

[simon-king-jena]

Changed work issues from Rebase rel 5.0.beta13 to none

[simon-king-jena]

comment:50

For the record: make test works fine (on x86_64 GNU/Linux), with #715, #11521, #12313, #11943 and #11935 applied on top of sage-5.0.beta13. Of course, I can't review the patch (hint-hint) ...

[nthiery]

Description changed:

--- 
+++ 
@@ -1,6 +1,16 @@
 At #11900 and [sage-combinat-devel](http://groups.google.com/group/sage-combinat-devel/browse_thread/thread/99c74827d704e677), 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. In the process, this patch also:
+
+- Adds a method Category._make_named_class providing a unified way to
+  create parent and element classes (and later on morphism classes)
+- Extends the interface of dynamic_class to customize caching and pickling
+
+

 __Apply__

-* [attachment: trac11935_weak_pickling_by_construction_rel11943.patch](https://github.com/sagemath/sage/files/ticket11935/trac11935_weak_pickling_by_construction_rel11943.patch.gz)
+* [attachment: trac11935_weak_pickling_by_construction_rel11943-nt.patch](https://github.com/sagemath/sage-prod/files/10653895/trac11935_weak_pickling_by_construction_rel11943-nt.patch.gz)

[nthiery]

Changed dependencies from #9138 #11900 #11943 to #9138, #11900, #11943, #12875, #12876, #12877

[nthiery]

comment:51

Hi Simon,

I reworked the patch by adding features to dynamic_class in order to avoid logic duplication and encapsulation breaking in make_named_class.

The downside is that this makes this ticket depend on #12876 (ensuring that parent/element classes are purely abstract).

All test should pass on 5.0.beta13, except for the two issues I mentionned in #12876. Oh, and one trivial failure I had forgotten in semigroup_cython.pyx. I'll update the patch later (tonight?) but you can start the review.

I folded the two patches to get a better overview. You can access the differential patch by looking up http://combinat.sagemath.org/patches/file/3121811e2ebe/trac11935_weak_pickling_by_construction_rel11943-review-nt.patch.

Cheers, Nicolas

[nthiery]

comment:52

Attachment: trac11935_weak_pickling_by_construction_rel11943-nt.patch.gz

Replying to @nthiery:

All test should pass on 5.0.beta13, except for the two issues I mentionned in #12876. Oh, and one trivial failure I had forgotten in semigroup_cython.pyx. I'll update the patch later (tonight?) but you can start the review.

The updated patch fixes the failure in semigroup_cython.pyx. If I am not mistaken, all tests pass on 5.0.beta14 (that is all failures I have seen should be due to the fact that I did not activate #715 in the Sage-Combinat queue, because it forces recompiling too much stuff).

Cheers, Nicolas

[nthiery]

Attachment: trac11935_share_on_base_category.patch.gz

[nthiery]

comment:53

Hi Simon,

While working on #12895, I got a non trivial time regression, due to the large number of constructed categories for which creating yet another *_class was non negligible. Investigating this with runsnake made me turn back to this ticket: too many categories are created for nothing.

It indeed sounds a bit like a waste, to construct the parent class of Algebras(GF(5)) to have to reconstruct all the hierarchy of super categories above Algebras(GF(5)) (e.g. Modules(GF(5)), ...). With the updated patch, Algebras(K).parent_class directly reuses Algebras(L).parent_class if it already exists and if K and L have the same category. The super_categories method of Algebras(K) is not even called.

To achieve this, each subcategory of CategoryWithParameters should provide a method _make_named_class_key specifying on what the parent_class (and friends) depend on. For example, Category_over_base specifies that parent_class depends only on the category of the base. Then, _make_named_class uses that to do a lookup in a cache.

For our typical benchmark:

%time L = EllipticCurve('960d1').prove_BSD()

the time on my machine goes from 4s down to 3.5s. With the subcategory patch, the times goes down from 7s to 3.75s. This makes the subcategory patch acceptable.

One fine point is that e.g. Algebras(ZZ) and Algebras(ZZ['x']) don't share the same parent class anymore, since ZZ and ZZ['x'] don't have the same category.

What do you think? Could you have a brief look at the experimental trac11935_share_on_base_category.patch I just attached? If it sounds reasonable to you, I'll finalize it (doctests, ...), and fold it in my reviewer's patch.

[simon-king-jena]

comment:54

Hi Nicolas!

What patches are supposed to be applied, currently? Only attachment: trac11935_weak_pickling_by_construction_rel11943-nt.patch (which I am now testing), or attachment: trac11935_share_on_base_category.patch as well?

[simon-king-jena]

comment:55

Replying to @nthiery:

It indeed sounds a bit like a waste, to construct the parent class of Algebras(GF(5)) to have to reconstruct all the hierarchy of super categories above Algebras(GF(5)) (e.g. Modules(GF(5)), ...). With the updated patch, Algebras(K).parent_class directly reuses Algebras(L).parent_class if it already exists and if K and L have the same category. The super_categories method of Algebras(K) is not even called.

I was thinking of that, too. But it would only work if we rely on the assumption that the list of super_categories of a category with base only depends on the category of the base. Can we? Then, to the very least, that assumption must be clearly stated somewhere.

To achieve this, each subcategory of CategoryWithParameters should provide a method _make_named_class_key specifying on what the parent_class (and friends) depend on. For example, Category_over_base specifies that parent_class depends only on the category of the base. Then, _make_named_class uses that to do a lookup in a cache.

For our typical benchmark:

%time L = EllipticCurve('960d1').prove_BSD()

the time on my machine goes from 4s down to 3.5s. With the subcategory patch, the times goes down from 7s to 3.75s. This makes the subcategory patch acceptable.

I am a bit confused. What is the "subcategory patch"? Is it "share_on_base_category"? And what patches are applied for the four different timings?

One fine point is that e.g. Algebras(ZZ) and Algebras(ZZ['x']) don't share the same parent class anymore, since ZZ and ZZ['x'] don't have the same category.

Sure, but I don't think that is necessarily bad.

What do you think? Could you have a brief look at the experimental trac11935_share_on_base_category.patch I just attached? If it sounds reasonable to you, I'll finalize it (doctests, ...), and fold it in my reviewer's patch.

I am currently running tests without it. But I am now reading it.

[simon-king-jena]

comment:56

Looking at the docs of sage.structure.dynamic_class, I see that the docs of DynamicClasscallMetaclass is broken. Shall I fix it here (in yet another reviewer patch) or leave it to a different ticket?

[nthiery]

comment:57

Replying to @simon-king-jena:

Hi Nicolas!

What patches are supposed to be applied, currently? Only attachment: trac11935_weak_pickling_by_construction_rel11943-nt.patch (which I am now testing), or attachment: trac11935_share_on_base_category.patch as well?

Both patches for the experimental feature of having the parent class depend only on the base ring.

[simon-king-jena]

comment:58

Apparently, the broken documentation of DynamicClasscallMetaclass comes from the init method of NestedClassMetaclass. So, it would be better to provide proper documentation. Just a short note.

While the tests are running, I noticed

sage -t  --long -force_lib devel/sage/sage/combinat/crystals/tensor_product.py
     [16.3 s]
sage -t  --long -force_lib devel/sage/sage/plot/complex_plot.pyx
     [21.5 s]
*** glibc detected *** python: double free or corruption (fasttop): 0x0000000003e99500 ***
sage -t  --long -force_lib devel/sage/sage/combinat/backtrack.py
     [15.1 s]

Where does that come from? Here, I work without the share_on_base_category.

[nthiery]

comment:59

Replying to @simon-king-jena:

I was thinking of that, too. But it would only work if we rely on the assumption that the list of super_categories of a category with base only depends on the category of the base. Can we?

I am indeed not sure about making that assumption for any Category_over_base (it is not clearly defined what a base is!). On the other hand, this seems quite reasonable to me for Category_over_base_ring. This makes e.g. Algebras(...) consistent with the other functorial constructions categories which depend only on the base category.

This also goes in the direction of what we had discussed that we could actually make Algebras(...) be a functorial construction, so that we could define C=Algebras(Fields()), and have Algebras(R) be basically an alias for C for every field. And similarly for PolynomialRings(Fields()), ...

Note that we could possibly change this ticket to leave Category_over_base alone, and have only Category_over_base_ring derive from CategoryWithParameters. Do you foresee examples of a plain Category_over_base where sharing parent classes would be important performance wise?

Then, to the very least, that assumption must be clearly stated somewhere.

YES

For our typical benchmark:

%time L = EllipticCurve('960d1').prove_BSD()

the time on my machine goes from 4s down to 3.5s. With the subcategory patch, the times goes down from 7s to 3.75s. This makes the subcategory patch acceptable.

I am a bit confused. What is the "subcategory patch"?

I meant #12895.

And what patches are applied for the four different timings?

With and without share_on_base_category and with and without #12895.

Cheers, Nicolas

[nthiery]

comment:60

Replying to @simon-king-jena:

Looking at the docs of sage.structure.dynamic_class, I see that the docs of DynamicClasscallMetaclass is broken. Shall I fix it here (in yet another reviewer patch) or leave it to a different ticket?

If it is a simple fix, go ahead.

[simon-king-jena]

comment:61

Apart from the strange glibc problem (that is not reported at the end of the test suite), I get one timeout, namely sage -t --long -force_lib devel/sage/sage/crypto/mq/mpolynomialsystem.py.

Aha! And it turns out that the glibc comes from that test!! Here is what I get in detail:

king@mpc622:/mnt/local/king/SAGE/stable/sage-5.0.beta13$ ./sage -t --verbose  -force_lib devel/sage/sage/crypto/mq/mpolynomialsystem.py
sage -t --verbose -force_lib "devel/sage/sage/crypto/mq/mpolynomialsystem.py"
Trying:
    set_random_seed(0L)
Expecting nothing
ok
Trying:
    change_warning_output(sys.stdout)
Expecting nothing
ok
Trying:
    sr = mq.SR(Integer(2),Integer(1),Integer(2),Integer(4),gf2=True,polybori=True)###line 26:_sage_    >>> sr = mq.SR(2,1,2,4,gf2=True,polybori=True)
Expecting nothing
ok
Trying:
    sr###line 27:_sage_    >>> sr
Expecting:
    SR(2,1,2,4)
ok
Trying:
    set_random_seed(Integer(1))###line 33:_sage_    >>> set_random_seed(1)
Expecting nothing
ok
Trying:
    F,s = sr.polynomial_system()###line 34:_sage_    >>> F,s = sr.polynomial_system()
Expecting nothing
*** glibc detected *** python: double free or corruption (fasttop): 0x0000000003b79b40 ***
^CAborting further tests.
KeyboardInterrupt -- interrupted after 2.3 seconds!

So, these are very few commands. That should be reproducible (trying it a bit later).

For reference: The problem does not occur with

trac_12808-classcall_speedup-fh.patch
trac_12808_nested_class_cython.patch
trac_12808-classcall_cdef.patch
trac12215_weak_cached_function.patch
trac12215_segfault_fixes.patch
trac_715_combined.patch
trac_11521_homset_weakcache_combined.patch
trac_12875-category-fix_abvar_homspace-nt.patch
trac_12877-category-for_more_rings_and_schemes-nt.patch
trac_12876_category-fix_abstract_class-nt-rel11521.patch
trac_12876-reviewer.patch
trac_12876_category-fix_abstract_class-nt-rel11521-review-nt.patch
trac9107_nesting_nested_classes.patch
trac11768_source_of_dynamic_class.patch
trac11768_docfix.patch
trac11817_question_mark_using_sage_getdoc.patch
trac11791_dynamic_metaclass_introspection.patch
trac11943_mro_for_all_super_categories_combined.patch

but does occur if one adds attachment: trac11935_weak_pickling_by_construction_rel11943-nt.patch.

[simon-king-jena]

comment:62

I can not reproduce the problem on the command line. Too bad.

sage: set_random_seed(0L)
sage: change_warning_output(sys.stdout)
---------------------------------------------------------------------------
NameError                                 Traceback (most recent call last)

/mnt/local/king/SAGE/stable/sage-5.0.beta13/devel/sage-main/<ipython console> in <module>()

NameError: name 'change_warning_output' is not defined
sage: sr = mq.SR(2,1,2,4,gf2=True,polybori=True)
sage: sr
SR(2,1,2,4)
sage: set_random_seed(1)
sage: F,s = sr.polynomial_system()

[simon-king-jena]

comment:63

I still can't reproduce the problem. Too bad. But I have another comment. With the patch, one has in sage/categories/category.py:

    def _make_named_class(...):
        ...
        else:
            # Otherwise, check XXXMethods
            import inspect
            assert inspect.isclass(method_provider_cls),\
                "%s.%s should be a class"%(type(self).__name__, method_provider)

That seems suboptimal to me:

sage: import inspect
sage: def test1(self, cls):
....:     import inspect
....:     assert inspect.isclass(cls),\
....:         "%s.%s should be a class"%(type(self).__name__, repr(cls))
....:
sage: def test2(self, cls):
....:     assert inspect.isclass(cls),\
....:         "%s.%s should be a class"%(type(self).__name__, repr(cls))
....:
sage: def test3(self, cls):
....:     if not inspect.isclass(cls):
....:         raise AssertionError, "%s.%s should be a class"%(type(self).__name__, repr(cls))
....:
sage: test2(ZZ,ZZ.__class__)
sage: %timeit test1(ZZ,ZZ.__class__)
625 loops, best of 3: 4.45 µs per loop
sage: %timeit test2(ZZ,ZZ.__class__)
625 loops, best of 3: 2.67 µs per loop
sage: %timeit test3(ZZ,ZZ.__class__)
625 loops, best of 3: 2.63 µs per loop

test1 is as in your code: inspect is imported, and (at least it seems to me) the error message is created even if the error does not occur. test2 does not import inspect again, while test3 is an attempt to not create the error message.

To my surprise, creating the error message seems to be essentially for free. But one should really import inspect top-level, I think.

[simon-king-jena]

comment:64

I wonder: Why is it tested in _make_named_class whether method_provider_cls is a class? Shouldn't that be the job of dynamic_class, which is called in the following lines?

[simon-king-jena]

comment:65

Since method_provider_cls is not being checked in dynamic_class, I guess it is fine to test it in _make_named_class.

I notice

        self._super_categories
        return dynamic_class(class_name,
                             tuple(getattr(cat,name) for cat in self._super_categories),
                             method_provider_cls, prepend_cls_bases = False, doccls = doccls,
                             reduction = (getattr, (self, name)), cache = cache)

i.e., first an empty call to self._super_categories. I guess that can be erased?

[simon-king-jena]

comment:66

Some other little speed-up:

sage: def test1(cls,name):
....:     s = "%s.%s"%(cls.__name__, name)
....:     
sage: def test2(cls,name):
....:     s = cls.__name__+'.'+name
....:     
sage: timeit("test1(ZZ.__class__,'element_class')", number=10000)
10000 loops, best of 3: 2.15 µs per loop
sage: timeit("test2(ZZ.__class__,'element_class')", number=10000)
10000 loops, best of 3: 1.85 µs per loop

So, adding strings seems to be faster than inserting into a format string.

[simon-king-jena]

comment:67

I just tested whether the changes I mentioned have a noticeable effect on the computation time for an example that does nothing but creating the parent classes for 2000 different categories (I made it so that the parent classes are distinct). I am afraid, it was not noticeable. So, I guess I can drop my suggestion.

[simon-king-jena]

comment:68

Concerning the glibc problem: I made no progress in tracking it down. I just know: When I trace all Python function calls then the problem vanishes, and when I temporarily disable garbage collection then it vanishes as well.

Hence, probably there is some object that is deallocated too early, which makes me wonder how it is related with the memleak fixes from #715, #11521 and #12215.

Nicolas, can you confirm the problem? You didn't mention it yet.

Also, I think I should try to test only the patches that are really needed: In addition to the dependencies (and the dependencies of the dependencies) of this ticket, I have #9107, #11768, #11817 and #11791 applied. I should test whether removing any of it would make the problem disappear.

[simon-king-jena]

comment:69

OK, removing the additional patches did not help.

[simon-king-jena]

comment:70

Hooray, #12215 is to blame (which doesn't have a positive review, yet). In #12215, dynamic classes become weakly cached.

The aim of #12215 is to avoid a memory leak that was partially caused by creating many different parent classes. Here, the same problem is solved in a different way, namely by avoiding that many different parent classes are created in the first place.

Hence, I am now trying whether it helps to have apply #12215, but reverting the cache of dynamic classes into a strong cache.

[simon-king-jena]

comment:71

Yippie! Returning to a strong cache for dynamic classes does indeed help! I am now running the test suite, because it is conceivable that the change makes part of the memleak re-appear.

[nthiery]

comment:72

Replying to @simon-king-jena:

That seems suboptimal to me: ... test1 is as in your code: inspect is imported, and (at least it seems to me) the error message is created even if the error does not occur. test2 does not import inspect again, while test3 is an attempt to not create the error message.

To my surprise, creating the error message seems to be essentially for free. But one should really import inspect top-level, I think.

According to: http://docs.python.org/reference/simple_stmts.html

assert expression1, expression2

is equivalent to:

if __debug__:
   if not expression1: raise AssertionError(expression2)

This means in particular that expression2 is not evaluated if expression1 does not hold. So we don't need to worry about efficiency in those.

That being said, yes, the ``import inspect'' should be at the toplevel! Thanks for catching this.

Are you in the process of writing a reviewer's patch, or do you want me to do the change? Note: I am flying to Montreal tomorrow, and my wife arrives in one hour from a two weeks mission; so you probably won't hear much from me until Sunday.

Cheers, Nicolas

[nthiery]

comment:73

Replying to @simon-king-jena:

Since method_provider_cls is not being checked in dynamic_class, I guess it is fine to test it in _make_named_class.

I notice

        self._super_categories
        return dynamic_class(class_name,
                             tuple(getattr(cat,name) for cat in self._super_categories),
                             method_provider_cls, prepend_cls_bases = False, doccls = doccls,
                             reduction = (getattr, (self, name)), cache = cache)

i.e., first an empty call to self._super_categories. I guess that can be erased?

Oh, yes. That is just a scory from a debugging session where I wanted to force the evaluation of the supercategories. Thanks.

[nthiery]

comment:74

Replying to @simon-king-jena:

Yippie! Returning to a strong cache for dynamic classes does indeed help! I am now running the test suite, because it is conceivable that the change makes part of the memleak re-appear.

Thanks for tracking this down! I don't have a preference for weak or strong caching dynamic classes.

[nthiery]

comment:75

Speaking of weak cache: currently trac11935_share_on_base_category.patch uses strong caching. Weak caching might be preferable.

Another thing is that this patch could be quite more concise if we had another feature in cached functions: namely that we could specify that certain arguments should be ignored in the cache lookup; or more generally that we could specify a function that would produce the key to be used in the cache lookup. Something like:

     def key(x, l, option=1): return x, tuple(l)

     @cached_function(key=key)
     def foo(x, l, option=1):
         return (x, l, option)

     sage: foo(1, [1,2], 3) is foo(1, (1,2), 2)
     True

Better syntax welcome!

Cheers, Nicolas

[nthiery]

comment:76

Replying to @simon-king-jena:

I just tested whether the changes I mentioned have a noticeable effect on the computation time for an example that does nothing but creating the parent classes for 2000 different categories (I made it so that the parent classes are distinct). I am afraid, it was not noticeable. So, I guess I can drop my suggestion.

Feel free to implement it anyway in a reviewer's patch. Addition is as readable, if not more, than using a format string.

[simon-king-jena]

comment:77

Hi Nicolas,

I was about to create a reviewer patch. But I am afraid the same problem creped back in sage/rings/polynomail/multi_polynomial_sequence.py. In other words: When I have a weak cache on dynamic classes, then the double free occurs in crypto/mq/mpolynomialsystem.py, but when I have a strong cache, then the double free occurs in rings/polynomial/multi_polynomial_sequence.py (but again in a test involving polynomial systems!).

I have a preference for a weak cache, and since apparently the weak cache as such is not to blame for the error, I need to look somewhere else. Perhaps there is some UniqueRepresentation in mpolynomialsystem.py that needs a strong cache. If that turns out to be correct, then I will do the necessary change in #12215.

Apart from that: I will be away until Monday as well.

[simon-king-jena]

comment:78

Too bad. Apparently the patch doesn't apply anymore (according to the patch bot). Apart from a possible rebasing: What needs to be done to get this behind us?

[simon-king-jena]

comment:79

Apparently a lot of things need to be done. Nicolas, could you remind me (and the patchbot) of the patches to be applied? I have just updated the patches at #12876, so that they apply on top of sage-5.3.beta2 plus dependencies.