Opened 4 years ago

Closed 7 months ago

#15947 closed enhancement (fixed)

Weaken types for _rmul_ and _lmul_

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-8.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 jdemeyer)

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 tscrim

  • Authors Travis Scrimshaw deleted
  • Cc nthiery nbruin vbraun SimonKing added

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.

comment:2 follow-up: Changed 4 years ago by 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.

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

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.

comment:4 Changed 4 years ago by tscrim

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

comment:5 Changed 4 years ago by SimonKing

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 tscrim

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.

Last edited 4 years ago by tscrim (previous) (diff)

comment:7 Changed 4 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:8 Changed 3 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:9 Changed 2 years ago by SimonKing

Is this perhaps superseded by #18756 and #18758?

comment:10 Changed 18 months ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-6.4 to sage-7.4

comment:11 Changed 18 months ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:12 follow-up: Changed 18 months ago by jdemeyer

  • Authors Jeroen Demeyer deleted
  • Description modified (diff)

comment:13 in reply to: ↑ 12 ; follow-up: Changed 18 months ago by SimonKing

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 18 months ago by jdemeyer

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 18 months ago by jdemeyer

  • Dependencies set to #21140

comment:16 Changed 8 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Milestone changed from sage-7.4 to sage-8.0

comment:17 Changed 8 months ago by jdemeyer

  • Branch set to u/jdemeyer/weaken_types_for__rmul__and__lmul_

comment:18 Changed 8 months ago by jdemeyer

  • 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 7 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:20 Changed 7 months ago by git

  • Commit changed from 8f294308f8b85b233ffdc9df3f20c404ee6431c9 to 30737e01947a7c687d951d65fb6ab486ffbd1aa2

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

30737e0Merge tag '8.0.beta11' into t/15947/weaken_types_for__rmul__and__lmul_

comment:21 Changed 7 months ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:22 Changed 7 months ago by tscrim

  • 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 7 months ago by vbraun

  • Branch changed from u/jdemeyer/weaken_types_for__rmul__and__lmul_ to 30737e01947a7c687d951d65fb6ab486ffbd1aa2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.