Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In *(scalar,Matrix), try promoting matrix in case promoting scalar fails #3186

Open
YueRen opened this issue Jan 15, 2024 · 9 comments
Open
Labels
enhancement New feature or request

Comments

@YueRen
Copy link
Member

YueRen commented Jan 15, 2024

Currently, multiplying a matrix by a scalar of different type only works if the scalar can be promoted to the type of the matrix:

julia> K,t = rational_function_field(QQ,"t")
(Rational function field over QQ, t)

julia> R,s = K["s"]
(Univariate polynomial ring in s over rational function field, s)

julia> A = matrix(R,[1 1 0 1; 1 0 1 1])
[1   1   0   1]
[1   0   1   1]

julia> A*3
[3   3   0   3]
[3   0   3   3]

It would be nice if - in case the scalar cannot be promoted - OSCAR tries promoting the matrix instead, so that the following multiplication returns a matrix over R:

julia> K,t = rational_function_field(QQ,"t")
(Rational function field over QQ, t)

julia> R,s = K["s"]
(Univariate polynomial ring in s over rational function field, s)

julia> A = matrix(K,[1 1 0 1; 1 0 1 1])
[1   1   0   1]
[1   0   1   1]

julia> A*s
ERROR: Cannot promote to common type
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] promote(x::AbstractAlgebra.Generic.MatSpaceElem{AbstractAlgebra.Generic.RationalFunctionFieldElem{QQFieldElem, QQPolyRingElem}}, y::AbstractAlgebra.Generic.Poly{AbstractAlgebra.Generic.RationalFunctionFieldElem{QQFieldElem, QQPolyRingElem}})                        
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/1K0hg/src/Matrix.jl:1049
 [3] *(x::AbstractAlgebra.Generic.MatSpaceElem{AbstractAlgebra.Generic.RationalFunctionFieldElem{QQFieldElem, QQPolyRingElem}}, y::AbstractAlgebra.Generic.Poly{AbstractAlgebra.Generic.RationalFunctionFieldElem{QQFieldElem, QQPolyRingElem}})                        
   @ AbstractAlgebra ~/.julia/packages/AbstractAlgebra/1K0hg/src/Matrix.jl:1058
 [4] top-level scope
   @ REPL[31]:1

Now that broadcasting works for OSCAR matrices, can we achieve this by simply redefining A*s as matrix(...,A.*s)?

@YueRen YueRen added the enhancement New feature or request label Jan 15, 2024
@YueRen
Copy link
Member Author

YueRen commented Jan 15, 2024

Btw., promotion of matrices works great in matrix addition (and multiplication). Oscar has no problems adding two matrices of different types as long as one can be promoted to the type of the other:

julia> K,t = rational_function_field(QQ,"t")
(Rational function field over QQ, t)

julia> A = matrix(K,[1 2; 3 4])
[1   2]
[3   4]

julia> B = matrix(QQ,[4 3; 2 1])
[4   3]
[2   1]

julia> A+B
[5   5]
[5   5]

julia> B+A
[5   5]
[5   5]

@thofma
Copy link
Collaborator

thofma commented Jan 15, 2024

Usually we do not do automatic promotion, where the parent of the result has to be constructed (like scalar times polynomial), but we should make an (additional) exception for matrices here.

@fingolfin
Copy link
Member

So on a tangent, I am not sure we can always do automatic conversion in this cases. To boot with, what is the definition of "a scalar" ? An element of a ring? So what if R is a matrix algebra, and I look at a matrix M over R (so a matrix whose entries are matrices) and I multiply M by a scalar a in R. Then I can view this as a scaler*matrix product. Or I could view a as a matrix, and then upcast it to a matrix over R, and perform matrix multiplication, which gives a different result. Maybe the latter is a bit artificial, but I wonder if we can write down explicitly rules to always make this fully unambiguous?

@thofma
Copy link
Collaborator

thofma commented Jan 17, 2024

I am inclined to allow this fancy stuff only for matrices constructed via matrix, and not the matrix ring elements. For the matrix ring/algebra elements, one has to jump through a lot of hoops to even construct them (e.g. A = zero(matrix_algebra(QQ, 5)). In this case, one usually knows what one is doing.

@JohnAAbbott
Copy link
Contributor

I think I understand @thofma comment about not automatically creating a parent -- in the example here the parent would presumably be matrix_space(R, 2,4). I'm not sure I understand @thofma comment immediately before this one I am writing.
We faced a similar question in CoCoA about automatic promotion (indeed even in the context scalar-times-matrix). I don't recall the final decision, if any. If automatic promotion does not work, is there a simple/neat/easy way to do the promotion manually, explicitly?

@YueRen
Copy link
Member Author

YueRen commented Jan 19, 2024

So on a tangent, I am not sure we can always do automatic conversion in this cases. To boot with, what is the definition of "a scalar" ? An element of a ring? So what if R is a matrix algebra, and I look at a matrix M over R (so a matrix whose entries are matrices) and I multiply M by a scalar a in R. Then I can view this as a scaler*matrix product. Or I could view a as a matrix, and then upcast it to a matrix over R, and perform matrix multiplication, which gives a different result. Maybe the latter is a bit artificial, but I wonder if we can write down explicitly rules to always make this fully unambiguous?

A quick first suggestion how to deal with a*b where a and b are different types would be:

  1. If both a and b are matrices, convert the entries of a and the entries of b.
  2. If say b is a matrix, but a is not, convert a and the entries of b.
  3. If neither a or b are matrices, convert a and b.

Here,

  • "matrix" means constructed via matrix (as @thofma suggested and avoiding the problem described by @fingolfin),
  • "convert" can either mean "convert to a common supertype" or "convert one to the type of the other".

@HereAround
Copy link
Member

I am inclined to allow this fancy stuff only for matrices constructed via matrix, and not the matrix ring elements. For the matrix ring/algebra elements, one has to jump through a lot of hoops to even construct them (e.g. A = zero(matrix_algebra(QQ, 5)). In this case, one usually knows what one is doing.

We discussed in triage today, and on that occasion there was no objection to this proposal by @thofma . So maybe this is the way forward with this.

@lkastner
Copy link
Member

This is similar to #2536 which was closed without changing anything.

@fingolfin fingolfin removed the triage label Dec 18, 2024
@thofma
Copy link
Collaborator

thofma commented Dec 25, 2024

The rules we propose already exist for matrix-vector operations:

julia> A * [s, s, s, s]
2-element Vector{AbstractAlgebra.Generic.Poly{AbstractAlgebra.Generic.RationalFunctionFieldElem{Rational{BigInt}, AbstractAlgebra.Generic.Poly{Rational{BigInt}}}}}:
 3*s
 3*s

I opened Nemocas/AbstractAlgebra.jl#1949 to add also the matrix-scalar rules we agreed on here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants