Opened 9 years ago

# Make parent/element classes independent of base rings — at Version 44

Reported by: Owned by: SimonKing nthiery major sage-5.11 categories parent class, element class, category jdemeyer, sage-combinat Simon King N/A #9138 #11900 #11943

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.

Apply

### comment:1 Changed 9 years ago by SimonKing

Concerning uniqueness of the parent class: In at least one case (namely `Algebras(R)`), the super categories depend on whether the base ring is a field or not. We would like to have

```sage: Algebras(ZZ).parent_class != Algebras(GF(3)).parent_class == Algebras(QQ).parent_class
True
```

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 follow-ups: ↓ 3 ↓ 4 Changed 9 years ago by SimonKing

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, `C.parent_class` is pickled by `getattr, (C,"parent_class")`. The pickling data (hence, C) is part of the cache key of a dynamic class. With that, the parent class of different categories C1 and C2 can't be the same.

I see three approaches to get rid of it.

1. Remove the pickling data from the cache key of dynamic classes
2. Make pickling of `C.parent_class` just rely on the default way of pickling a dynamic class
3. Work around the cache of dynamic classes, but still use `getattr,(C,"parent_class")` for pickling.

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.

1. means:

The pickle of a parent class will only depend on the category graph as it was on the time of pickling. If the category graph changes between pickling and unpickling the parent class, you would get a different class.

1. would be a bit more stable.

The idea is:

(i) In the lazy attribute `parent_class()`, the dynamic class is first created without providing the reduction data (as in approach 2.). (ii) Before returning that dynamic class, it is tested whether the reduction data is still none. If it is, the `getattr, (C,"parent_class")` thingy is inserted.

Consequence: `Algebras(QQ).parent_class` could, for example, be unpickled as `Algebras(GF(2)).parent_class` - which is not a big problem, since we want them to be the same. However, if in a distant future we want them to be different again, we'd be in trouble...

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 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 9 years ago by nthiery

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":

True: all parents ought to be pickled "by construction" rather than by "class + internal data", in order to encapsulate as much as possible of the data structure. This probably ought to be true as well for elements. I don't know how far we are from this.

A good thing to do at this point would be to search through the sage pickle jar for how many parent_class's and element_class's are pickled there. And why. I don't know how complicated it is to do this search though.

Among the three propositions, I like 3 best. I have trouble evaluating how big the risks are to get stuck in the future. It does not seem too big.

Thanks Simon for investigating this!

### comment:4 in reply to: ↑ 2 Changed 9 years ago by SimonKing

• Description modified (diff)
• Summary changed from Make parent/element classes independent of base rings and the category graph consistent with method resolution to Make parent/element classes independent of base rings

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.

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 in reply to: ↑ 3 ; follow-up: ↓ 8 Changed 9 years ago by SimonKing

A good thing to do at this point would be to search through the sage pickle jar for how many parent_class's and element_class's are pickled there.

Old pickles will not break, I believe. Let `P3 = Algebras(GF(3)).parent_class` and `PQ = Algebras(QQ).parent_class`. Here are a few scenarios:

1. P3 and PQ have been created and pickled with an old version of Sage

In the old version of Sage, `P3!=PQ`. They are pickled by construction. Hence, after applying my yet-to-be-submitted patches, they are unpickled as `Algebras(GF(3)).parent_class` and `Algebras(QQ).parent_class` - which is the same class after applying the patch.

Conclusion: An old pickle will not break, with either approach 2. or 3. The worst what could happen is `P3!=PQ` before pickling and `P3==PQ` after unpickling. But the defining property `P3==Algebras(GF(3)).parent_class` and `PQ==Algebras(QQ).parent_class` is preserved.

2. P3 and PQ are created and pickled according to approach 2. from above

Of course, `P3==PQ` at the time of pickling. The pickle will only depend on the parent classes of the super categories of `Algebras(GF(3))` and `Algebras(QQ)`. If there was a change in the super categories between pickling and unpickling, we would have `P3!=Algebras(GF(3)).parent_class` after unpickling.

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 `P3==Algebras(GF(3)).parent_class`.

3. P3 and PQ are created and pickled according to approach 3. from above

Of course, `P3==PQ` at the time of pickling. `PQ` and `P3` will both be pickled by construction. In particular, a change in the category graph would not matter, as long as the super categories of `Algebras(QQ)` and `Algebras(GF(3))` still coincide (up to the base ring) after the change in the category graph.

A problem would arise if, in a distant future, the super categories of `Algebras(QQ)` and `Algebras(GF(3))` would become essentially different. Say, some algebra person finds that vector spaces over fraction fields should have their own category, different from the usual category of vector spaces. Then, `Algebras(QQ).parent_class!=Algebras(GF(3)).parent_class`. In particular, after such change, unpickling `P3` or `PQ` would result in either `PQ!=Algebras(QQ).parent_class` or `P3!=Algebras(GF(3)).parent_class`.

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 `P3==Algebras(GF(3)).parent_class` and `PQ==Algebras(QQ).parent_class` after unpickling. However, if the super categories will depend on the base ring in a different way as it is now, then either P3 or PQ will lose the defining property after unpickling, while the other will keep the defining property - and we don't know which of the two will break.

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.

### Changed 9 years ago by SimonKing

Use default pickling for parent/element classes, making them base ring independent.

### comment:6 Changed 9 years ago by SimonKing

• Authors set to Simon King
• Status changed from new to needs_review

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 `C.ParentMethods` and on the parent classes of the super categories of C, but it will only depend on the base ring of C if the base ring changes the super categories (which holds for algebras, e.g.).

Note the effect on the computation time for elliptic curves. With sage-4.7.2.alpha3 plus #11900, we have

```sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 3.97 s, sys: 0.07 s, total: 4.04 s
Wall time: 4.18 s
```

but with the new patch on top of it, we have

```sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 3.11 s, sys: 0.06 s, total: 3.17 s
Wall time: 3.31 s
```

### Changed 9 years ago by SimonKing

Use a weak form of "pickling by construction" for parent and element classes of categories

### comment:7 Changed 9 years ago by SimonKing

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 `Algebras(QQ).parent_class`, then its cache key as a dynamic class only comprises the parent classes of the super categories. Before returning it, the unpickling data (by construction) is added. When `Algebras(GF(3)).parent_class` is created later, it is found in the cache of dynamic classes and immediately returned. The class returned will thus be unpickled as `Algebras(QQ).parent_class`.

Similar to the first patch, it considerably speeds up the elliptic curve computations:

```sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 3.05 s, sys: 0.07 s, total: 3.12 s
Wall time: 3.27 s
```

Apply only one of the two patches, at your choice!

### comment:8 in reply to: ↑ 5 ; follow-up: ↓ 9 Changed 9 years ago by nthiery

A good thing to do at this point would be to search through the sage pickle jar for how many parent_class's and element_class's are pickled there.

Old pickles will not break, I believe.

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!

It seems to me that approach 3. is less fragile than 2.

+1

Let's see if someone else has some preference between the two implementations.

Cheers,

Nicolas

### comment:9 in reply to: ↑ 8 ; follow-up: ↓ 10 Changed 9 years ago by SimonKing

What I wanted to know whether currently most parents and elements were pickled by construction or by "class + data".

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 in reply to: ↑ 9 ; follow-up: ↓ 11 Changed 9 years ago by nthiery

What I wanted to know whether currently most parents and elements were pickled by construction or by "class + data".

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.

Yup. Or run explain_pickle on all pickles, and grep for element_class / parent_class.

### comment:11 in reply to: ↑ 10 Changed 9 years ago by SimonKing

Yup. Or run explain_pickle on all pickles, and grep for element_class / parent_class.

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 Changed 9 years ago by SimonKing

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 `<class 'matplotlib.font_manager.FontEntry'>`, `<class 'ZODB.fsIndex.fsIndex'>` and `<class 'sage.misc.explain_pickle.EmptyNewstyleClass'>`.

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:13 Changed 9 years ago by jdemeyer

• Dependencies changed from #11900 to #9138, #11900

### comment:14 Changed 9 years ago by SimonKing

• Description modified (diff)

`sage -testall -long` passes for either patch. So, unless we will find bad surprises in the pickle jar, both approaches should be fine. I am slightly in favour of approah 3.

### comment:15 Changed 9 years ago by SimonKing

• Dependencies changed from #9138, #11900 to #9138 #11900 #11943
• Description modified (diff)

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

### comment:16 follow-up: ↓ 17 Changed 9 years ago by SimonKing

• Status changed from needs_review to needs_work
• Work issues set to Fix doctest in covariant functorial construction

For some reason, #11935 together with #11943 result in one doctest error, namely in the "FooBars?" test of `sage/categories/covariant_functorial_construction.py`. So, it needs work.

### comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 9 years ago by SimonKing

For some reason, #11935 together with #11943 result in one doctest error, namely in the "FooBars?" test of `sage/categories/covariant_functorial_construction.py`. So, it needs work.

Here is the problem:

```sage: from sage.categories.covariant_functorial_construction import CovariantConstructionCategory
sage: class FooBars(CovariantConstructionCategory):
...       _functor_category = 'FooBars_function'
...
sage: def FooBars_function(X): return FooBars(X)
...
sage: C = FooBars(ModulesWithBasis(QQ))
sage: import __main__; __main__.FooBars_function = FooBars_function
sage: __main__.FooBars = FooBars
sage: Category.FooBars_function = FooBars_function
```

Now, requesting `C.parent_class` results in a complaint regarding "`duplicate base class FooBars.parent_class`". Indeed, with the patch from here, we have

```sage: CA = FooBars(AdditiveMagmas())
sage: CM = FooBars(Magmas())
sage: CA.parent_class == CM.parent_class
True
```

even though we have

```sage: Magmas().parent_class is AdditiveMagmas().parent_class
False
```

So, I was overdoing it: With my patch, the parent class not only becomes independent of a base ring, but also it becomes independent of the base category of a covariant functorial construction - and this is not what we want.

The point is that CA and CM above have the same super categories, namely `FooBars(Sets())`. But with my patch, parent classes are the same if both the super categories and the `ParentMethods` are the same. The `ParentMethods` are different for `Magmas()` and `AdditiveMagmas()`, but they are the same for `FooBars(Magmas())` and `FooBars(AdditiveMagmas())`.

I wonder how this should be solved.

One possibility is that sage.categories.covariant_functorial_constructions overrides the parent_class and element_class lazy attributes that are defined in sage.categories.category.

### comment:18 in reply to: ↑ 17 ; follow-up: ↓ 19 Changed 9 years ago by nthiery

Hi Simon!

So, I was overdoing it: With my patch, the parent class not only becomes independent of a base ring, but also it becomes independent of the base category of a covariant functorial construction - and this is not what we want.

The point is that CA and CM above have the same super categories, namely `FooBars(Sets())`. But with my patch, parent classes are the same if both the super categories and the `ParentMethods` are the same. The `ParentMethods` are different for `Magmas()` and `AdditiveMagmas()`, but they are the same for `FooBars(Magmas())` and `FooBars(AdditiveMagmas())`.

I wonder how this should be solved.

One possibility is that sage.categories.covariant_functorial_constructions overrides the parent_class and element_class lazy attributes that are defined in sage.categories.category.

Ah ah, interesting challenge!

So, the question is whether we want to put in the specifications:

Constraint (C1): above any category in the category hierarchy, there should be a one-to-one correspondence between categories and their parent class. In particular, C.all_super_categories() and C.parent_class.mro() should exactly match.

For C(...) a parametrized categories, let me call (O) the optimization of having C(...).parent_class depend only on C.ParentMethods? and the parent class of the super categories of C(...).

If we want (C1), then we indeed have to be careful with parametrized categories. In particular, it seems to me (I haven't written a proof though :-)) that optimization (O) can only be used for a parametrized category C(...) if it is further guaranteed that:

Constraint (C2): no parent is simultaneously in two instances C(A) and C(B) of C.

(C2) seems reasonable for a Category_over_base_ring (a parent should have a unique distinguished base ring). Maybe it would make sense as well for a Category_over_base. Your example above shows that we don't want it in general, and in particular not for covariant constructions.

This calls for Category to not implement (O) by default, and Category_over_base_ring to override parent_class and element_class to implement (O).

Another option would be to drop (C1) altogether: it seems like a nice optimization to reduce the number of classes in the mro whenever possible (e.g. when categories do not provide ParentMethods?). But then we face an interesting poset/lattice problem, namely whether the C3 algorithm is (can be made) compatible with taking certain subposets.

This is quite related to the kind of optimizations I do in #10963, so I'd love to have a brainstorm about that; but we'd better have it face to face when you come over. In any cases, I vote for imposing (C1) in this ticket, and think about removing this constraint, if at all possible, in a later ticket. Both for not making this ticket even more complicated and reducing conflicts with #10963.

Cheers,

Nicolas

### comment:19 in reply to: ↑ 18 Changed 9 years ago by SimonKing

Hi Nicolas,

If we want (C1), then we indeed have to be careful with parametrized categories. In particular, it seems to me (I haven't written a proof though :-)) that optimization (O) can only be used for a parametrized category C(...) if it is further guaranteed that:

Constraint (C2): no parent is simultaneously in two instances C(A) and C(B) of C.

(C2) seems reasonable for a Category_over_base_ring (a parent should have a unique distinguished base ring). Maybe it would make sense as well for a Category_over_base.

That would indeed be a possibility. My first reaction was "The problem arose in covariant functorial construction, thus, use (O) by default, but let covariant functorial constructions override it with the non-optimised approach".

But after all, the subject of this ticket is "Make parent/element classes independent of base rings". So, your suggestion fits 100% into the scope of this ticket.

However, it is not all that easy. For example, by #9138, the category of a univariate polynomial ring is a JOIN of a category with base (namely commutative algebras) and a base free category (Euclidean domains or so). The join category has a `base()` method, due to one of the patches at #11900, but it does not inherit from `sage.categories.category_types.Category_over_base`.

In other words:

If we use optimisation (O) only on `sage.categories.category_types.Category_over_base` then we would not see any speedup in elliptic curve computations.

Here are a few scenarios:

1. Use (O) by default, but do not use it on covariant functorial constructions. The question is whether this is consistent.
2. Do not use (O) by default, but use it on `Category_over_base`. Problem: We would not see a speed-up.
3. Do not use (O) by default, but use it on every category that has a `base` method. That includes `Category_over_base`, but also (by #11900) any category that is sub-category of a category with base.

The problem with the third approach is: Testing whether the category is sub-category of a category with base is expensive. Thus, it is possible that the regression (due to this test) is bigger than the speed-up obtained from optimisation (O).

### comment:20 Changed 9 years ago by jdemeyer

• Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

### comment:21 follow-ups: ↓ 22 ↓ 23 Changed 9 years ago by SimonKing

• Milestone set to sage-4.8

I think the following could be a solution:

1. Do not use optimization for `Category.parent_class`. Hence, the default is the good old "pickle by construction" approach.
2. Add a specialised `JoinCategory.parent_class` that uses default pickling of a dynamic class (which means: The class is uniquely determined by the base classes). Rationale: A join category is uniquely determined by its super categories, and thus it is consequent if the parent class of a join category is uniquely determined by the parent classes of its super categories.
3. Add a specialised `Category_over_base.parent_class` using the optimization (O) discussed above, in the "weak pickling by construction" abbroach. Rationale: It's the purpose of this ticket to make the parent class independent of the base ring, and "weak pickling by construction" seems the most stable option.

Apparently, the problem with functorial constructions would vanish - they use the non-optimized old parent_class. But we would get a speed-up where we need it: Polynomial rings belong to a join category, and one super category of that join category is a `Category_over_base`.

Of course, the same should be done with the element_class.

### comment:22 in reply to: ↑ 21 Changed 9 years ago by SimonKing

1. Add a specialised `Category_over_base.parent_class` using the optimization (O) discussed above, in the "weak pickling by construction" approach.

... and to `Bimodules` as well. They have two bases, but they do not inherit from 'Category_over_base'.

### comment:23 in reply to: ↑ 21 ; follow-up: ↓ 24 Changed 9 years ago by nthiery

I think the following could be a solution:

1. Do not use optimization for `Category.parent_class`. Hence, the default is the good old "pickle by construction" approach.
2. Add a specialised `JoinCategory.parent_class` that uses default pickling of a dynamic class (which means: The class is uniquely determined by the base classes). Rationale: A join category is uniquely determined by its super categories, and thus it is consequent if the parent class of a join category is uniquely determined by the parent classes of its super categories.
3. Add a specialised `Category_over_base.parent_class` using the optimization (O) discussed above, in the "weak pickling by construction" abbroach. Rationale: It's the purpose of this ticket to make the parent class independent of the base ring, and "weak pickling by construction" seems the most stable option.

Apparently, the problem with functorial constructions would vanish - they use the non-optimized old parent_class. But we would get a speed-up where we need it: Polynomial rings belong to a join category, and one super category of that join category is a `Category_over_base`.

Of course, the same should be done with the element_class.

This sounds very good.

Just a suggestion: in Category, you may want to put a lazy attribute _parent_class_from_base_classes, so that the categories that want the optimization (like Modules or Bimodules) can just do parent_class = _parent_class_from_base_classes (same for element_class of course)

parent_class

### comment:24 in reply to: ↑ 23 Changed 9 years ago by SimonKing

Just a suggestion: in Category, you may want to put a lazy attribute _parent_class_from_base_classes, so that the categories that want the optimization (like Modules or Bimodules) can just do parent_class = _parent_class_from_base_classes (same for element_class of course)

Excellent idea!

### comment:25 Changed 9 years ago by SimonKing

The patch is not yet ready for publication, but in `L = EllipticCurve('960d1').prove_BSD()` I see a speedup of nearly 25% compared with #11900+#11943!

So, according to your suggestion, I add a _parent_class_from_bases and _element_class_from_bases to Category, and use it for categories over base and for bimodules.

However, there is a slight problem: You can not simply define `parent_class = _parent_class_from_bases` if you want to have a real lazy attribute. Namely, `parent_class` would believe that its name is `_parent_class_from_bases`:

```sage: class Foo(object):
....:     @lazy_attribute
....:     def _bar(self):
....:         return 5
....:     bar = _bar
....:
sage: f = Foo
sage: f.bar.__name__
'_bar'
```

In particular, it is not as fast as it should be. With the not-submitted patch:

```sage: C = Modules(GF(3),dispatch=False)
sage: %timeit p = C.parent_class
625 loops, best of 3: 69.4 µs per loop
sage: %timeit p = C._parent_class_from_bases
625 loops, best of 3: 440 ns per loop
```

But it might be possible to work around.

### comment:26 follow-up: ↓ 29 Changed 9 years ago by SimonKing

Concerning lazy attributes: I wonder whether one could add a method `rename(name)` to a lazy attribute. That method would return a copy of the original lazy attribute, but with a new name.

Then, the example above would become

```sage: class Foo(object):
....:     @lazy_attribute
....:     def _bar(self):
....:         return 5
....:     bar = _bar.rename("bar")
....:
sage: f = Foo
sage: f.bar.__name__
'bar'
```

### comment:27 Changed 9 years ago by SimonKing

• Dependencies changed from #9138 #11900 #11943 to #9138 #11900 #11943 #11999

I created #11999 for the possibility to rename lazy attributes, and make it a dependency.

### comment:28 follow-up: ↓ 30 Changed 9 years ago by SimonKing

• Status changed from needs_work to needs_review
• Work issues Fix doctest in covariant functorial construction deleted

Done!

The current patch preserves the default parent_class and element_class for categories. In particular, there is no problem with the covariant functorial constructions.

According to Nicolas' idea, I added a _parent_class_from_bases and _element_class_from_bases. They use the "weak pickling-by-construction" approach discussed above, because that seems to minimize the probability of breaking new pickles in a distant future. Just to emphasize: Old pickles will still work.

These two new lazy attributes override parent_class and element_class for `Category_over_base` (which was made possible by #11999). By consequence, the parent classes of all vector spaces (over different base fields) coincide. The parent classes of algebras over fields coincide, but differ from the parent class of algebras over a non-field.

Moreover, the parent class of a join category only depends on the parent classes of its super categories. Here, I use the default pickling of dynamic classes.

Rationale for using the default pickling in the case of join categories:

• The creation of a dynamic class becomes slightly faster when you don't need to worry about pickling.
• A join category is uniquely determined by its super categories. So, it is safe to make the parent class uniquely determined by the bases (which is precisely the default pickling of dynamic classes).

With the new patch, all long tests pass. Moreover, I have the following timing:

```sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 2.95 s, sys: 0.08 s, total: 3.03 s
Wall time: 3.20 s
```

That is a speed-up of about 20% compared with unpatched sage-4.7.2.alpha2!

Apply trac11935_weak_pickling_by_construction_rel11943.patch

### comment:29 in reply to: ↑ 26 ; follow-up: ↓ 31 Changed 9 years ago by nthiery

Concerning lazy attributes: I wonder whether one could add a method `rename(name)` to a lazy attribute. That method would return a copy of the original lazy attribute, but with a new name.

Then, the example above would become

```sage: class Foo(object):
....:     @lazy_attribute
....:     def _bar(self):
....:         return 5
....:     bar = _bar.rename("bar")
....:
sage: f = Foo
sage: f.bar.__name__
'bar'
```

For what's its worth, a potential variant:

```sage: class Foo(object):
....:     @lazy_attribute(name="bar")
....:     def _bar(self):
....:         return 5
....:     bar = _bar.rename("bar")
....:
sage: f = Foo
sage: f.bar.__name__
'bar'
```

In our use case, we want the lazy attribute parent_class_from_bases to be called parent_class whenever it's used by subclasses of Category.

Cheers,

Nicolas

### comment:30 in reply to: ↑ 28 Changed 9 years ago by nthiery

With the new patch, all long tests pass. Moreover, I have the following timing:

```sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 2.95 s, sys: 0.08 s, total: 3.03 s
Wall time: 3.20 s
```

That is a speed-up of about 20% compared with unpatched sage-4.7.2.alpha2!

Congrats!

### comment:31 in reply to: ↑ 29 ; follow-up: ↓ 32 Changed 9 years ago by SimonKing

For what's its worth, a potential variant:

```sage: class Foo(object):
....:     @lazy_attribute(name="bar")
```

You mean: Add an optional argument "name" to the lazy attribute? I was thinking about that, too.

Using it means that (in the example above) `f._bar` would result in assigning `f.__dict__["bar"] = 5` (note: "bar", not "_bar", even though the lazy attribute is called requested as "_bar").

So, when we do

```@lazy_attribute(name="parent_class")
def _parent_class_from_bases(self):
return bla
```

then the following would happen, where C is a category:

• `C._parent_class_from_bases` would write its result into `C.parent_class`, thus, potentially overriding an already existing parent class. Do we want that such a dangerous thing can happen?
• On the bright side, we could easily do
```class Foo(Category):
parent_class = _parent_class_from_bases
```
without renaming, because the name is already right.

If you think this should be added, I can easily do so on #11999.

### comment:32 in reply to: ↑ 31 ; follow-up: ↓ 33 Changed 9 years ago by nthiery

You mean: Add an optional argument "name" to the lazy attribute? I was thinking about that, too.

Yup.

Using it means that (in the example above) `f._bar` would result in assigning `f.__dict__["bar"] = 5` (note: "bar", not "_bar", even though the lazy attribute is called requested as "_bar").

Hmm, this smells indeed. I am not sure. At this point, I am wondering if we don't want instead to introduce a new subclass:

```    class CategoryWithClassesFromBases(Category):  # TODO: find a better name
```

with the two optimized parent_class / element_class (and possibly in the future morphism_class / category_class), and have:

```    class Category_over_base_ring(CategoryWithClassesFromBases): ...
class JoinCategory(CategoryWithClassesFromBases): ...
```

Cheers,

Nicolas

### comment:33 in reply to: ↑ 32 ; follow-up: ↓ 34 Changed 9 years ago by SimonKing

Hi Nicolas,

class CategoryWithClassesFromBases?(Category): # TODO: find a better name

Perhaps `CategoryEnsemble`? The name is short and refers to the fact that the members of an ensemble (such as all categories of algebras over fields) are sufficiently similar that they share important features (such as their parent class).

Or (slightly longer) `CategoryWithParameters`? Again, if you have parameters (such as one or two base rings) then still certain important features may not depend on the parameters. Or `ParametrizedCategory`, but I think you prefer if the name starts with the word "Category", isn't it?

class JoinCategory?(CategoryWithClassesFromBases?): ...

I think join categories should have their own parent class. Reason:

The parent/element classes of a category ensemble are pickled by a weak form of "pickling by construction": If C1,C2,... belong to an ensemble of categories that share their parent class, then that class will be pickled as getattr(C,'parent_class'), where C is any member of the ensemble.

But I think that `JoinCategory` should not use that "pickling by weak construction". Reason: For `JoinCategory`, the "pickling by weak construction" is equivalent to the default pickling of dynamic classes (which is: the class is determined by name, bases, and potentially a new class such as `ParentMethods`, which is empty in the case of a `JoinCategory`). Hence, it would be a waste of time to construct the pickle data for the `JoinCategory` while they are created.

I will certainly test both approaches. But if I remember correctly what I did yesterday, the difference between "pickling by weak construction" or "default pickling" for join categories was 6% in the infamous elliptic curve benchmark.

### comment:34 in reply to: ↑ 33 Changed 9 years ago by SimonKing

• Dependencies changed from #9138 #11900 #11943 #11999 to #9138 #11900 #11943

I have updated my patch according to what we have discussed.

• I added a subclass `CategoryWithParameters` of the `Category` class. Of course, if you have a shorter yet more descriptive name, I can change that. I believe `CategoryWithClassesFromBases` is too long and not clearer than `CategoryWithParameters`. And I think `CategoryEnsemble` is not clear either.
• Both `Category_over_base`, `JoinCategory` and `Bimodules` inherit from the new class.
• Pickling is by weak construction: A parent class P is pickled by `getattr, (C,'parent_class')`, where C is any category such that C.parent_class is P at the time of pickling. We had discussed advantages and disadvantages of this and other approaches.

Using the new class, the patch becomes independent of #11999. Of course, I still think that #11999 is a nice addition, but your suggestion to use a sub-class is better.

I will certainly test both approaches. But if I remember correctly what I did yesterday, the difference between "pickling by weak construction" or "default pickling" for join categories was 6% in the infamous elliptic curve benchmark.

Here I was mistaken: With the new patch, the benchmark becomes

```sage: %time L = EllipticCurve('960d1').prove_BSD()
CPU times: user 2.87 s, sys: 0.05 s, total: 2.92 s
Wall time: 3.10 s
```

and this is as fast as by using default pickling for parent classes of join categories.

Apply trac11935_weak_pickling_by_construction_rel11943.patch

### comment:35 Changed 9 years ago by SimonKing

Sorry, it seems that I had forgotten to `hg qrefresh`. Now, the patch is really updated...

Apply trac11935_weak_pickling_by_construction_rel11943.patch

### comment:36 Changed 9 years ago by SimonKing

Gosh, where is my brain? The previous patch did still contain references to the `_parent_class_from_bases`. It is now removed. But I think I need a break.

Apply trac11935_weak_pickling_by_construction_rel11943.patch

### comment:37 Changed 9 years ago by SimonKing

• Status changed from needs_review to needs_work
• Work issues set to introduce _make_named_class

Some suggestion of Nicolas: The creation of the parent_class and of the element_class follow the same logic: We have the corresponding classes of the super categories, take them as bases, and add some methods that are available in the attribute `ParentMethods` or `ElementMethods`.

So, it seems reasonable to write a new method that implements that logic. Then, parent_class and element_class would both simply call that method. Originally, we suggested the name `_make_member_class` for that method, because it creates a class for some member (object, element of object, or in future morphism) of the category.

But meanwhile I prefer the name `_make_named_class`, because the parameter is indeed a name. So, roughly like this:

```class Category:
...
def _make_named_class(self, name, methods_holder):
the default logic

@lazy_attribute
def parent_class(self):
return self._make_named_class("parent_class", "ParentMethods")
@lazy_attribute
def element_class(self):
return self._make_named_class("element_class", "ElementMethods")
```

Then, `CategoryWithParameters` only needs to override one thing, namely `_make_named_class`.

### Changed 9 years ago by SimonKing

Refactor parent/element class creation: Use _make_named_class

### comment:38 Changed 9 years ago by SimonKing

• Description modified (diff)
• Status changed from needs_work to needs_review
• Work issues introduce _make_named_class deleted

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

### comment:39 Changed 9 years ago by SimonKing

• Status changed from needs_review to needs_work
• Work issues set to Rebase wrt the new version of #11900; ideas behind Category_singleton need to be modified if parent classes are shared

### comment:40 Changed 9 years ago by SimonKing

• Status changed from needs_work to needs_review
• Work issues Rebase wrt the new version of #11900; ideas behind Category_singleton need to be modified if parent classes are shared deleted

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

### comment:41 Changed 9 years ago by SimonKing

FWIW: Tests pass.

### comment:42 Changed 8 years ago by nthiery

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

### comment:43 Changed 8 years ago by nthiery

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.

### comment:44 Changed 8 years ago by nthiery

• Description modified (diff)
Note: See TracTickets for help on using tickets.