Opened 4 years ago
Closed 9 months ago
#15947 closed enhancement (fixed)
Weaken types for _rmul_ and _lmul_
Reported by:  tscrim  Owned by:  tscrim 

Priority:  major  Milestone:  sage8.0 
Component:  coercion  Keywords:  
Cc:  nthiery, nbruin, vbraun, SimonKing  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  30737e0 (Commits)  Commit:  30737e01947a7c687d951d65fb6ab486ffbd1aa2 
Dependencies:  #21140  Stopgaps: 
Description (last modified by )
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
.
Change History (23)
comment:1 Changed 4 years ago by
 Cc nthiery nbruin vbraun SimonKing added
comment:2 followup: ↓ 3 Changed 4 years ago by
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.
comment:3 in reply to: ↑ 2 Changed 4 years ago by
Replying to tscrim:
Oh, here's an idea. What about having the
ElementMethods
forAlgebras
inherit fromAlgebraElement
? 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.
comment:4 Changed 4 years ago by
But here we would have an intermediate python class. Do you remember if that helped at all?
comment:5 Changed 4 years ago by
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
.
comment:6 Changed 4 years ago by
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 nontrivial 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 categorybased 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.
comment:7 Changed 4 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:8 Changed 4 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:9 Changed 3 years ago by
comment:10 Changed 20 months ago by
 Description modified (diff)
 Milestone changed from sage6.4 to sage7.4
comment:11 Changed 20 months ago by
comment:12 followup: ↓ 13 Changed 20 months ago by
 Description modified (diff)
comment:13 in reply to: ↑ 12 ; followup: ↓ 14 Changed 20 months ago by
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
.
comment:14 in reply to: ↑ 13 Changed 20 months ago by
Replying to SimonKing:
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.
comment:15 Changed 20 months ago by
 Dependencies set to #21140
comment:16 Changed 10 months ago by
 Milestone changed from sage7.4 to sage8.0
comment:17 Changed 10 months ago by
 Branch set to u/jdemeyer/weaken_types_for__rmul__and__lmul_
comment:18 Changed 10 months ago by
 Commit set to 8f294308f8b85b233ffdc9df3f20c404ee6431c9
 Status changed from new to needs_review
New commits:
8f29430  _lmul_ and _rmul_: scalar should be Element instead of RingElement

comment:19 Changed 9 months ago by
 Status changed from needs_review to needs_work
comment:20 Changed 9 months ago by
 Commit changed from 8f294308f8b85b233ffdc9df3f20c404ee6431c9 to 30737e01947a7c687d951d65fb6ab486ffbd1aa2
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_

comment:21 Changed 9 months ago by
 Status changed from needs_work to needs_review
comment:22 Changed 9 months ago by
 Reviewers set to Travis Scrimshaw
 Status changed from needs_review to positive_review
Patchbot (essentially) green and this is something that we should do. Positive review and hoping that there are no conflicts.
comment:23 Changed 9 months ago by
 Branch changed from u/jdemeyer/weaken_types_for__rmul__and__lmul_ to 30737e01947a7c687d951d65fb6ab486ffbd1aa2
 Resolution set to fixed
 Status changed from positive_review to closed
The problem begins with
_rmul_()
and_lmul_()
in (cython) matrices/vectors/etc. take aRingElement
as an argument. The issue is that not all elements in rings inherit fromRingElement
(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:
So converting things to use
CombinatorialFreeModule
from old parents (in particular,FreeAlgebra
) breaks things that use to work.