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

Addition/subtraction of Matrix{Sym} and UniformScaling gives Matrix{Any} #359

Closed
olof3 opened this issue Jun 8, 2020 · 5 comments
Closed

Comments

@olof3
Copy link

olof3 commented Jun 8, 2020

@vars a b
typeof([a 1; b b] + I)

gives Matrix{Any} and not Matrix{Sym}

Multiplication seems to work though.

Not sure if the problem is with SymPy.jl or that the logic in LinearAlgebra is too fragile?

@jverzani
Copy link
Collaborator

jverzani commented Jun 8, 2020

Thanks, it comes down to

Base._return_type(+, (Sym, typeof(I))) #  Any
Base._return_type(*, (Sym, typeof(I)))  # UniformScaling

I'm not sure how to hint that the first return type should be Sym. Do you have any idea? That would be the right fix. Otherwise, we could just define through A .+ I.λ.

@olof3
Copy link
Author

olof3 commented Jun 8, 2020

Nope, I don't really have any good ideas, I'm afraid.

eltype(symbols("a") + I) gives Sym and since that seems to work I feel like it should be possible to infer the correct type so not sure what is broken. The code for the type inference is a bit daunting to me.

Note sure what you mean by A .+ I.λ? I guess A .+ λI != A .+ I.λ

@jverzani
Copy link
Collaborator

jverzani commented Jun 8, 2020

Yes, I'm not sure what is broken or how to fix the promotion. As you see, but unfortunately Base._return_type(+, (eltype(symbols("a")), eltype(I))) is checked and that gives Any. I think adding the method

Base.:+(A::AbstractMatrix, I::UniformScaling) = (n=LinearAlgebra.checksquare(A); A .+ I.λ*I(n))

will work as desired, as I.λ*I(n) creates an object that dispatches differently than I alone (the way λ*I is stored allows the scalar to be found through I.λ).

jverzani added a commit to jverzani/SymPy.jl that referenced this issue Jun 10, 2020
jverzani added a commit that referenced this issue Jun 13, 2020
* close issue #359; tweak travis

* test newer versions only

* tweak twice
@olof3
Copy link
Author

olof3 commented May 10, 2021

Ran into this problem again, when the arguments were ordered differently. I guess a similar fix as previously should be alright.

@syms a
A1 = [1 a; a 1]
# The first six cases work
@test typeof(A1 + a*I) == Matrix{Sym}
@test typeof(A1 - a*I) == Matrix{Sym}
@test typeof(-A1 + a*I) == Matrix{Sym}
@test typeof(-A1 - a*I) == Matrix{Sym}
@test typeof(-a*I + A1) == Matrix{Sym}
@test typeof(a*I + A1) == Matrix{Sym}
# The following two fails, returning Matrix{Any}
@test typeof(a*I - A1) == Matrix{Sym}
@test typeof(-a*I - A1) == Matrix{Sym}

# All of the following fails, returning Matrix{Any}
A2 = [1 2; 2 1]
@test typeof(A2 + a*I) == Matrix{Sym}
@test typeof(A2 - a*I) == Matrix{Sym}
@test typeof(-A2 + a*I) == Matrix{Sym}
@test typeof(-A2 - a*I) == Matrix{Sym}
@test typeof(-a*I + A2) == Matrix{Sym}
@test typeof(a*I + A2) == Matrix{Sym}
@test typeof(a*I - A2) == Matrix{Sym}
@test typeof(-a*I - A2) == Matrix{Sym}

jverzani added a commit to jverzani/SymPy.jl that referenced this issue May 10, 2021
@jverzani
Copy link
Collaborator

Thanks for the report! This is hopefully handled for good with the new PR which just adds more methods to catch the dispatch to the fallback.

jverzani added a commit that referenced this issue May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants