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

cholfact and bkfact fail for non-<:StridedMatrix types #18972

Closed
Sacha0 opened this issue Oct 16, 2016 · 2 comments · Fixed by #47081
Closed

cholfact and bkfact fail for non-<:StridedMatrix types #18972

Sacha0 opened this issue Oct 16, 2016 · 2 comments · Fixed by #47081
Labels
Hacktoberfest Good for Hacktoberfest participants linear algebra Linear algebra

Comments

@Sacha0
Copy link
Member

Sacha0 commented Oct 16, 2016

Similar to #18970, cholfact and bkfact fail for matrices that are not subtypes of StridedMatrix. Contrast with qrfact, which succeeds for many more matrix types. (The difference seems to be that qrfact has a fallback that gracefully handles <:AbstractArrays more generally. Fixing this may be as simple as adding such ~five-line fallbacks for bkfact and cholfact.) Best!

julia> VERSION
v"0.6.0-dev.979"

julia> bkfact(SymTridiagonal(fill(2, 4), ones(3)))
ERROR: MethodError: no method matching bkfact(::SymTridiagonal{Float64})
Closest candidates are:
  bkfact(::Union{Hermitian{T,S},Symmetric{T,S}}) at linalg/symmetric.jl:183
  bkfact{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64}}(::Union{Base.ReshapedArray{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64},2,A<:DenseArray,MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N}}},DenseArray{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64},2},SubArray{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64},2,A<:Union{Base.ReshapedArray{T,N,A<:DenseArray,MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N}}},DenseArray},I<:Tuple{Vararg{Union{Base.AbstractCartesianIndex,Colon,Int64,Range{Int64}},N}},L}}) at linalg/bunchkaufman.jl:72
  bkfact{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64}}(::Union{Base.ReshapedArray{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64},2,A<:DenseArray,MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N}}},DenseArray{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64},2},SubArray{T<:Union{Complex{Float32},Complex{Float64},Float32,Float64},2,A<:Union{Base.ReshapedArray{T,N,A<:DenseArray,MI<:Tuple{Vararg{Base.MultiplicativeInverses.SignedMultiplicativeInverse{Int64},N}}},DenseArray},I<:Tuple{Vararg{Union{Base.AbstractCartesianIndex,Colon,Int64,Range{Int64}},N}},L}}, ::Symbol) at linalg/bunchkaufman.jl:72
  ...

(Yes, LDLt would be better for [edit: positive- or quasi-definite] SymTridiagonals.) (Edited to include cholfact.)

@Sacha0 Sacha0 added the linear algebra Linear algebra label Oct 16, 2016
@Sacha0 Sacha0 changed the title bkfact fails for non-<:StridedMatrix types cholfact and bkfact fail for non-<:StridedMatrix types Oct 16, 2016
@tkelman
Copy link
Contributor

tkelman commented Oct 17, 2016

LDLt would be better for SymTridiagonal

Many indefinite cases won't necessarily admit a diagonal-D factorization, will they? Though I think once you need Bunch-Kaufman pivoting then you can't necessarily remain within a fixed number of subdiagonals unless you use the more recent banded retraction factorization, IIRC.

@Sacha0
Copy link
Member Author

Sacha0 commented Oct 18, 2016

Many indefinite cases won't necessarily admit a diagonal-D factorization, will they? Though I think once you need Bunch-Kaufman pivoting then you can't necessarily remain within a fixed number of subdiagonals unless you use the more recent banded retraction factorization, IIRC.

Good point --- I had only the positive- and quasi-definite cases in mind. Best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Good for Hacktoberfest participants linear algebra Linear algebra
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants