use julias ishermitian instead of CHOLMOD's

[KristofferC]

No point in coupling this file and CHOLMOD when we have a julia implementation of ishermitian. This is also faster. Not that it matters compared to the time in factorizing the matrix.

[andreasnoack]

The Sparse constructor checks for symmetry/Hermitianity on construction so this would trigger two checks. It should be fine if you wrap the matrix in the Hermitian type, i.e. cholfact(Hermitian(A)). That would bypass the second check.

[KristofferC]

AC is not used anymore in this function after being checked for hermitian so I don't see how this would create any extra checks. Unless AC is supposed to be used in cholfact instead of A.

[andreasnoack]

As I read the code

julia/base/sparse/cholmod.jl

Line 882 in fe23b88

if ishermitian(o)

makes a check and so does cholfact in

julia/base/sparse/cholmod.jl

Line 1319 in fe23b88

kws...) = cholfact(Sparse(A); kws...)

by calling Sparse.

[KristofferC]

Yes, so then it is already checking symmetry twice, no? Once in AC = CHOLMOD.Sparse(A) and once in cholfact(A). Note that it is not cholfact(AC) in the current code.

[andreasnoack]

then it is already checking symmetry twice, no?

Kind of, but the check is extremely cheap, see

julia/base/sparse/cholmod.jl

Lines 1543 to 1545 in fe23b88

	if s.stype != 0
	return true
	else

In any case, I think it would make sense to bypass the second check in your change by using Hermitian.

[KristofferC]

I think we are talking past each other.

My point is that we are not sending in the created CHOLMOD.Sparse to cholfact. Immediately after ishermitian(AC) has been run, AC can be deleted because it is no longer used anywhere. We are instead sending in the original matrix A into cholfact where CHOLMOD.Sparse is created again exactly where you linked.

Concretely, shouldn't it be cholfact(A) -> cholfact(AC) in the current code to avoid the double creation of the CHOLMOD.Sparse object?

[andreasnoack]

Concretely, shouldn't it becholfact(A) -> cholfact(AC) in the current code to avoid the double creation of the CHOLMOD.Sparse object?

I missed that. Indeed, it should have been AC in cholfact. However, I still think it is safe to avoid the second symmetri check by using Hermitian. Do you see an issue with that?

[KristofferC]

Do you see an issue with that?

Not at all, just wanted to make sure I didn't have a misunderstanding, :)

Just doing Hermitian(A) does not work

julia> cholfact(Hermitian(A))
ERROR: MethodError: no method matching cholfact(::Hermitian{Float64,SparseMatrixCSC{Float64,Int64}}

so I would need to do something like cholfact(CHOLMOD.Sparse(Hermitian(A)) which makes the new code worse than before. I updated the code to just use the AC object consistently.

[andreasnoack]

Just doing Hermitian(A) does not work

See

julia/base/sparse/cholmod.jl

Lines 1317 to 1318 in fe23b88

	Symmetric{T,SparseMatrixCSC{T,SuiteSparse_long}},
	Hermitian{Complex{T},SparseMatrixCSC{Complex{T},SuiteSparse_long}}};

.

Right now it has to be Symmetric to work for real matrices but the signature should probably just be extended to cover Hermitian{Float64,A}. It would be annoying to branch on<:Real` here.

[KristofferC]

Something like this then?

[andreasnoack]

Yes. Exactly.