cholesky accessor

[rofinn]

Adds support for a cholesky accessor method where appropriate. Related to JuliaStats/Distributions.jl#731.

NOTE: Tests currently fail locally because of JuliaLinearAlgebra/Arpack.jl#12

[rofinn]

Alright, the tests pass locally for me, but the PDSparseMat test is kinda gross now.

[nickrobinson251]

How do you feel about adding a method for PDiagMat to this PR too? :)

[oxinabox]

bump?

[rofinn]

NOTE: I've added support for PDiagMat, but that technically isn't an accessor anymore.

[davidanthoff]

And another bump, this would be really nice to have.

[davidanthoff]

Actually, maybe also add it for ScalMat?

[rofinn]

@davidanthoff Should that return the cholesky of a Diagonal from the ScalMat?

[davidanthoff]

I would think so? But @andreasnoack probably knows better.

Now that I looked a bit more carefully at the package, I'm actually slightly confused. Why isn't AbstractPDMat a subtype of AbstractMatrix? EDIT: But I think that question shouldn't stop this PR from being merged. I see that we already have an issue for this question: #40.

[rofinn]

Thanks @andreasnoack and @davidanthoff!

[davidanthoff]

Thank you!

[davidanthoff]

Oh, and one more thing: @andreasnoack could you also tag a new release? Thanks :)

[johnczito]

Should we say in the README that cholesky should be one of the necessary methods for customized subtypes? If not, are folks open to a generic fallback like this?

cholesky(A::AbstractPDMat) = cholesky(Matrix(A))

[davidanthoff]

The more I think about this, the more I think AbstractPDMat should just derive from AbstractMatrix, in which case I would assume everything would just work automatically. Accessor methods like the one in this PR here then would be performance improvements, but I would assume that the functionality would just be there (saying all of this under the assumption that there is a cholesky that works for any AbstractMatrix, which I haven't actually checked).

[andreasnoack]

So, unfortunetely, this broke support for Julia builds with USE_GPL_LIBS=0 since PDSparseMat is only defined when SuiteSparse is available and is now used unconditionally on

PDMats.jl/src/PDMats.jl

Line 42 in 78cb506

const HAVE_CHOLMOD = isdefined(SuiteSparse, :CHOLMOD)

. @rofinn Would you be able to submit a fix? Eventually, it would also be good to test this with CI but it probably requires that we build Julia from source which I don't think is feasible.

[rofinn]

I'm not sure I follow. How is it being used unconditionally now? Oh, is this because testutils is in src/ rather than test/?

[andreasnoack]

The problem is that the struct PDSparseMat isn't defined when USE_GPL_LIBS=0 but it is referenced in

PDMats.jl/src/testutils.jl

Line 33 in 78cb506

isa(C, Union{PDMat, PDSparseMat, PDiagMat}) && t_cholesky && pdtest_cholesky(C, Cmat, cmat_eq, verbose)

[ararslan]

Potential solution:

const PDMatType = if HAVE_CHOLMOD
    Union{PDMat, PDiagMat, PDSparseMat}
else
    Union{PDMat, PDiagMat}
end

Then change the check to isa(C, PDMatType)