Weaken types for _rmul_ and _lmul_

[tscrim]

As a step towards removing the old parent with gens and using categories, we need to have _rmul_ and _lmul_ take any Element, not just RingElement.

Depends on #21140

CC: @nthiery @nbruin @vbraun @simon-king-jena

Component: coercion

Author: Jeroen Demeyer

Branch/Commit: 30737e0

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/15947

[tscrim]

Changed author from Travis Scrimshaw to none

[tscrim]

comment:1

The problem begins with _rmul_() and _lmul_() in (cython) matrices/vectors/etc. take a RingElement as an argument. The issue is that not all elements in rings inherit from RingElement (in fact, the category framework says we shouldn't have to do this). So the _lmul_() and _rmul_() error out and subsequently the attempt for coercion.

However, more and more I'm convincing myself that my proposal is not the way to do things unless we want Element to have a default _mul_. Yet we want to potentially let the categories give such an implementation. I'm not quite sure what the best course of action is. Thoughts?

The problem can be seen with the following:

sage: m = SymmetricFunctions(QQ).m()
sage: M = matrix(m, [[m[1], m[2]],[m[1,1], m[1]]])
sage: M
[   m[1]    m[2]]
[m[1, 1]    m[1]]
sage: m[1,1] * M
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-5-e031e70d2f91> in <module>()
----> 1 m[Integer(1),Integer(1)] * M

/home/travis/sage/local/lib/python2.7/site-packages/sage/categories/algebras.pyc in __mul__(self, right)
    203             from sage.structure.element import get_coercion_model
    204             import operator
--> 205             return get_coercion_model().bin_op(self, right, operator.mul)
    206 
    207 #        __imul__ = __mul__

/home/travis/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:8374)()

/home/travis/sage/local/lib/python2.7/site-packages/sage/structure/coerce.so in sage.structure.coerce.CoercionModel_cache_maps.bin_op (sage/structure/coerce.c:8262)()

TypeError: unsupported operand parent(s) for '*': 'Symmetric Functions over Rational Field in the monomial basis' and 'Full MatrixSpace of 2 by 2 dense matrices over Symmetric Functions over Rational Field in the monomial basis'

So converting things to use CombinatorialFreeModule from old parents (in particular, FreeAlgebra) breaks things that use to work.

[tscrim]

comment:2

Oh, here's an idea. What about having the ElementMethods for Algebras inherit from AlgebraElement? This would give us the benefit of using the strongly typed speed of c(ython) but still using the category framework. I can do experiments if people think this is a good idea.

[simon-king-jena]

comment:3

Replying to @tscrim:

Oh, here's an idea. What about having the ElementMethods for Algebras inherit from AlgebraElement? This would give us the benefit of using the strongly typed speed of c(ython) but still using the category framework. I can do experiments if people think this is a good idea.

I do, and in fact at some point I tried to use more of the "good old" Cython base classes in the category framework.

One problem, if I recall correctly, is a bug in Cython (still not fixed if I recall correctly) that makes it possible to create a Python class C that inherits from two Cython base classes A, B, such that some methods of A trying to access cdef attributes of A get confused by cdef attributes of B. So, the Cython does not recognise that the class layouts are incompatible.

[tscrim]

comment:4

But here we would have an intermediate python class. Do you remember if that helped at all?

[simon-king-jena]

comment:5

I don't remember. I think the problem was to simultaneously inherit from ModuleElement and Map. So, not uncommon.

Just imagine that you let Modules(R).element_class inherit from ModuleElement (which sounds reasonable), let Sets().hom_category().element_class inherit from Map (which makes sense, too), and then you might be in trouble with VectorSpaces(F).hom_category().element_class.

[tscrim]

comment:6

So because of the mechanism for constructing element classes, I can't OOTB just add AlgebraElement and have that be a part of the MRO. If I change the mechanism, then it leads to a segfault (which admittedly I'm too lazy to figure out as they seemed non-trivial to solve). It seems like the best hack/patch solution is to have CombinatorialFreeModuleElement do the same abuse as matrices and inherit from AlgebraElement. With this change we get:

sage: C = CombinatorialFreeModule(QQ, ['a','b','c'])
sage: a,b,c = C.basis()
sage: a*b
...
TypeError: unsupported operand parent(s) for '*': 'Free module generated by {'a', 'b', 'c'} over Rational Field' and 'Free module generated by {'a', 'b', 'c'} over Rational Field'
sage: h = SymmetricFunctions(QQ).h()
sage: m = matrix(h, [[h[2,1], h[3,2]], [h[1], h[1]]])
sage: h[2,1] * m
[h[2, 2, 1, 1] h[3, 2, 2, 1]]
[   h[2, 1, 1]    h[2, 1, 1]]

Although at some point we will need a category-based approach to this problem, but I don't see how to do so

Although I've come across an independent bug with 1x1 matrices:

sage: matrix(h, [[h[2,1]]])
...
AttributeError: 'tuple' object has no attribute 'parent'

I can post branches/commits of the various attempts I mentioned above too.

[simon-king-jena]

comment:9

Is this perhaps superseded by #18756 and #18758?

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

Changed author from Jeroen Demeyer to none

[simon-king-jena]

comment:13

Replying to @jdemeyer:

I think you are right, "any object" would be too broad. If you wanna implement an algebraic structure, you should at least inherit from Element.

[jdemeyer]

comment:14

Replying to @simon-king-jena:

I think you are right, "any object" would be too broad.

That was not the reason: "any object" does not work since code often needs the parent of the second argument of _lmul_ (and only an Element has a parent).

Unlike _mul_, it seems that the parents of the arguments of _lmul_ and _rmul_ are not defined.

If you wanna implement an algebraic structure, you should at least inherit from Element.

True, but besides the point. How your algebraic structure is implemented does not say anything about the API of calling _lmul_.

I have an eventual goal in mind of merging all the various *Element classes which will then automatically solve this ticket too.

[jdemeyer]

Dependencies: #21140

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

Branch: u/jdemeyer/weaken_types_for__rmul__and__lmul_

[jdemeyer]

Commit: 8f29430

[jdemeyer]

New commits:

8f29430

_lmul_ and _rmul_: scalar should be Element instead of RingElement

[sagetrac-git]

Changed commit from 8f29430 to 30737e0

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. New commits:

30737e0

Merge tag '8.0.beta11' into t/15947/weaken_types_for__rmul__and__lmul_

[tscrim]

comment:22

Patchbot (essentially) green and this is something that we should do. Positive review and hoping that there are no conflicts.

[tscrim]

Reviewer: Travis Scrimshaw

[vbraun]

Changed branch from u/jdemeyer/weaken_types_for__rmul__and__lmul_ to 30737e0