JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.65k stars 5.48k forks source link

cholfact documentation incorrectly claims pivoting is on by default #21762

Closed fiatflux closed 7 years ago

fiatflux commented 7 years ago

With reference to 0.5 Linear Algebra documentation, "pivoting is on by default" but it does not appear that is true. For the record, I do not think pivoting should be on by default for Cholesky.

The following code fragment prints "Caught exception; no pivoting used in default." on Version 0.5.0 (2016-09-19 18:14 UTC). I know this is an old build but a quick search suggests this hasn't changed in 0.5.2; sorry if I'm wrong about that.

A = Symmetric(rand(5,5)+eye(5))
default_lfact = cholfact(A)[:L]
pivoted_lfact = cholfact(A, Val{true})[:L]
unpivoted_lfact = cholfact(A, Val{false})[:L]
@assert default_lfact == unpivoted_lfact
@assert default_lfact != pivoted_lfact
try
    cholfact(Symmetric(zeros(2,2)))
catch
    println("Caught exception; no pivoting used in default.")
end
fiatflux commented 7 years ago

Oh, I now notice that portion of the docs is for sparse cholesky. Perhaps it could help others to clarify that a bit.

kshyatt commented 7 years ago

I agree that a clarification would probably be helpful. @fiatflux would you feel up to making a PR to fix this? Your perspective as someone got caught by this would be useful!

fredrikekre commented 7 years ago

The offending docstring starts with the following:

Compute the Cholesky factorization of a sparse positive definite matrix `A`.
`A` must be a `SparseMatrixCSC`, `Symmetric{SparseMatrixCSC}`, or
`Hermitian{SparseMatrixCSC}`. [...]

Not sure how it could be clarified. Add type annotations to the signature?

"""
    cholfact(A::SparseMatrixCSC; shift = 0.0, perm = Int[]) -> CHOLMOD.Factor
    cholfact(A::Symmetric{SparseMatrixCSC}; shift = 0.0, perm = Int[]) -> CHOLMOD.Factor
    cholfact(A::Hermitian{SparseMatrixCSC}; shift = 0.0, perm = Int[]) -> CHOLMOD.Factor

Compute the Cholesky factorization of a sparse positive definite matrix `A`.
`A` must be a `SparseMatrixCSC`, `Symmetric{SparseMatrixCSC}`, or
`Hermitian{SparseMatrixCSC}`.
andreasnoack commented 7 years ago

I think it is pretty clear. @fiatflux if you think it can be improved then please open a PR.