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

no method for A*A' when A = LowerTriangular(Diagonal(...)) #556

Closed
tpapp opened this issue Aug 24, 2018 · 9 comments
Closed

no method for A*A' when A = LowerTriangular(Diagonal(...)) #556

tpapp opened this issue Aug 24, 2018 · 9 comments

Comments

@tpapp
Copy link
Contributor

tpapp commented Aug 24, 2018

julia> VERSION
v"0.7.1-pre.0"

julia> using LinearAlgebra

julia> A = LowerTriangular(Diagonal([1, 2]))
2×2 LowerTriangular{Int64,Diagonal{Int64,Array{Int64,1}}}:
 1  
 0  2

julia> A * A'
ERROR: MethodError: no method matching rmul!(::SparseArrays.SparseMatrixCSC{Int64,Int64}, ::Adjoint{Int64,LowerTriangular{Int64,Diagonal{Int64,Array{Int64,1}}}})
@tpapp
Copy link
Contributor Author

tpapp commented Aug 24, 2018

BTW, I am afraid that having matrices like above comes from my own JuliaLang/julia#27823, eg

julia> using LinearAlgebra; cholesky(inv(Diagonal([1.0, 4.0]))).L
2×2 LowerTriangular{Float64,Diagonal{Float64,Array{Float64,1}}}:
 1.0    
 0.0  0.5

@tpapp
Copy link
Contributor Author

tpapp commented Sep 4, 2018

I have been thinking about this issue, and broadly I can think of two solutions:

  1. relax the requirement that getpropery(::Cholesky, ...) should always return an ::AbstractTriangular,

  2. make Diagonal <: AbstractTriangular, as it technically is.

Also, can someone please add the linear algebra label to the issue?

@tpapp
Copy link
Contributor Author

tpapp commented Oct 26, 2018

Friendly ping: I would be happy to make a PR fixing this, just need some guidance on the preferred solution. @andreasnoack, can you please help with this?

@andreasnoack
Copy link
Member

I think it would be unfortunate to lose the diagonal structure which could easily happen if the Diagonal is hidden behind LowerTriangular. Hence, I think we should change getproperty to dispatch on the wrapped array type and return Diagonal when the wrapped type is Diagonal.

@dkarrasch
Copy link
Member

@tpapp Do you still think we need this method, or was the issue kind of resolved with the diagonal cholesky decomposition?

@tpapp
Copy link
Contributor Author

tpapp commented Nov 11, 2020

The original example still fails for me on master.

@dkarrasch
Copy link
Member

dkarrasch commented Nov 11, 2020

The original example still fails for me on master.

Sure. But this is a very peculiar combination. Would you expect that one to occur in the wild?

@laborg
Copy link

laborg commented Feb 11, 2022

@tpapp do you think this can be closed now?

julia> versioninfo()
Julia Version 1.8.0-DEV.1456
Commit 10ded70514* (2022-02-05 07:49 UTC)
...

julia> using LinearAlgebra

julia> A = LowerTriangular(Diagonal([1, 2]))
2×2 LowerTriangular{Int64, Diagonal{Int64, Vector{Int64}}}:
 1  
 0  2

julia> A * A'
2×2 Matrix{Int64}:
 1  0
 0  4

@laborg laborg closed this as completed Feb 20, 2022
@tpapp
Copy link
Contributor Author

tpapp commented Feb 20, 2022

@laborg: sorry for not responding earlier. Yes, I think this has been fixed somewhat accidentally by JuliaLang/julia#43127 (git bisect confirms this). I will make some a PR with a test so that this remains so.

tpapp referenced this issue in tpapp/julia Feb 21, 2022
for the issue (and a trivial typo fix).
ViralBShah referenced this issue in JuliaLang/julia Feb 21, 2022
for the issue (and a trivial typo fix).
staticfloat referenced this issue in JuliaCI/julia-buildkite-testing Mar 2, 2022
for the issue (and a trivial typo fix).
LilithHafner referenced this issue in LilithHafner/julia Mar 8, 2022
for the issue (and a trivial typo fix).
@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
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

5 participants