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

evalpoly is broken for matrices #1161

Open
LilithHafner opened this issue Jan 1, 2025 · 9 comments · May be fixed by #1163
Open

evalpoly is broken for matrices #1161

LilithHafner opened this issue Jan 1, 2025 · 9 comments · May be fixed by #1163
Labels
bug Something isn't working

Comments

@LilithHafner
Copy link
Member

help?> evalpoly
evalpoly(x, p)

Evaluate the polynomial \sum_k x^{k-1} p[k] ...

julia> _evalpoly(x, p) = sum(x^(k-1)*p[k] for k in eachindex(p))
_evalpoly (generic function with 1 method)

julia> x = 10*one(zeros(2,2))
2×2 Matrix{Float64}:
 10.0   0.0
  0.0  10.0

julia> p = [1,2,3]
3-element Vector{Int64}:
 1
 2
 3

julia> _evalpoly(x, p)
2×2 Matrix{Float64}:
 321.0    0.0
   0.0  321.0

julia> evalpoly(x, p)
ERROR: MethodError: no method matching +(::Matrix{Float64}, ::Int64)
For element-wise addition, use broadcasting with dot syntax: array .+ scalar
The function `+` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...)
   @ Base operators.jl:596
  +(::BigFloat, ::Union{Int16, Int32, Int64, Int8})
   @ Base mpfr.jl:506
  +(::Base.CoreLogging.LogLevel, ::Integer)
   @ Base logging/logging.jl:132
  ...

Stacktrace:
 [1] muladd(x::Matrix{Float64}, y::Int64, z::Int64)
   @ Base.Math ./math.jl:1470
 [2] _evalpoly
   @ ./math.jl:114 [inlined]
 [3] evalpoly(x::Matrix{Float64}, p::Vector{Int64})
   @ Base.Math ./math.jl:107
 [4] top-level scope
   @ REPL[27]:1
@LilithHafner LilithHafner added the bug Something isn't working label Jan 1, 2025
@stevengj
Copy link
Member

stevengj commented Jan 1, 2025

Also, in a separate code path, for evalpoly(zeros(2,2), (1,2,3))

@jishnub
Copy link
Collaborator

jishnub commented Jan 1, 2025

Seems like a Base issue where it is trying to add a number to a matrix? Perhaps evalpoly may add p[i]*J instead of p[i], where J is an appropriate identity element. I'm unsure which one to use in general: perhaps oneunit?

@stevengj
Copy link
Member

stevengj commented Jan 1, 2025

It's just a matter of how the sum is initialized — it should be initialized by one(x) * p[end] rather than by p[end].

(Note that it should be one, not oneunit, because the former corresponds to x^0.)

@stevengj
Copy link
Member

stevengj commented Jan 1, 2025

However, I experimented with this and it seems like we need a completely separate method when x is a matrix and the coefficients are scalars, because of #1162 — even if you fix the sum initialization, it gives completely wrong results because of the weird behavior of muladd.

(Also, a specialized method for matrix x could be a good idea anyway because it could work more in-place.)

So, for now, the fact that it throws an error is a good thing — better than giving the wrong answer.

@stevengj
Copy link
Member

stevengj commented Jan 2, 2025

Here is a generic implementation that we can use as a fallback, which should handle most matrix types AFAICT:

# non-inplace fallback
function _evalpoly(X::AbstractMatrix, p)
    Base.require_one_based_indexing(p)
    p0 = isempty(p) ? Base.reduce_empty_iter(+, p) : p[end]
    Xone = one(X)
    S = Base.promote_op(*, typeof(Xone), typeof(Xone))(Xone) * p0
    for i = length(p)-1:-1:1
        S = X * S + @inbounds(p[i] isa AbstractMatrix ? p[i] : p[i] * I)
    end
    return S
end

evalpoly(X::AbstractMatrix, p::Tuple) = _evalpoly(X, p)
evalpoly(X::AbstractMatrix, p::AbstractVector) = _evalpoly(X, p)

It would be nice to have an optimized implementation when p is an array or tuple of scalars that works in-place, since such matrix polynomial functions are pretty common, but it's a little tricky to decide what types of X it should handle. Just X::StridedMatrix{<:Number}, maybe? (Otherwise you can easily run into trouble for things like SMatrix or other exotic types.)

@LilithHafner
Copy link
Member Author

A generic in place version could work using similar. However, in the case of SMatrix it may sometimes be more performant to use an out of place version that returns an SMatrix than an inplace version that returns a heap allocated MMatrix. I think that your proposed generic fallback is worth merging.

@stevengj
Copy link
Member

stevengj commented Jan 3, 2025

A generic in place version could work using similar.

You can't just use similar(X), because for some matrix types (e.g. SymTridiagonal) the type of X * X is different from that of X or similar(X) — that's why I used Base.promote_op in the code above. I guess you could call the promote_op conversion on similar(X), though this might possibly make an extra copy. It also may be tricky to get in-place code that works well for sparse matrix types that don't support efficient random mutation.

I also agree that for SMatrix you would probably want the out-of-place version.

@LilithHafner
Copy link
Member Author

I was thinking of S = Base.promote_op(similar ∘ *, typeof(Xone), typeof(Xone))(Xone) * p0

@stevengj
Copy link
Member

stevengj commented Jan 3, 2025

You also run into trouble for matrices of matrices, but I guess those aren't well-supported in linear algebra anyway right now. e.g. one(X) throws a MethodError for X = [rand(2,2) for i=1:2, j=1:2], even though in principle this should be fine.

@stevengj stevengj linked a pull request Jan 3, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants