Update some wording in the docstring of cholmod.jl

[WalterMadelim]

https://discourse.julialang.org/t/what-is-a-recommended-way-to-solve-a-linear-equation/128420/31?u=waltermadelim

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.95%. Comparing base (b225a85) to head (33eb31f).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #622   +/-   ##
=======================================
  Coverage   83.95%   83.95%           
=======================================
  Files          12       12           
  Lines        9265     9265           
=======================================
  Hits         7778     7778           
  Misses       1487     1487           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ViralBShah]

Thanks for the suggestions, but the original wording is better.

[WalterMadelim]

When I read the existing docstring, I feel that the docstring tells me that it is invalid to do the following, which is actually valid.

julia> A = [10 1; 2 10]
2×2 Matrix{Int64}:
 10   1
  2  10

julia> tagged_A = Symmetric(A)
2×2 Symmetric{Int64, Matrix{Int64}}:
 10   1
  1  10

julia> cholesky(tagged_A)
Cholesky{Float64, Matrix{Float64}}
U factor:
2×2 UpperTriangular{Float64, Matrix{Float64}}:
 3.16228  0.316228
  ⋅       3.14643

The existing docstring has a tone to urge me to do

julia> A = Matrix(Symmetric(A)) # you must first do this
2×2 Matrix{Int64}:
 10   1
  1  10

julia> cholesky(Symmetric(A)) # then you can do this
Cholesky{Float64, Matrix{Float64}}
U factor:
2×2 UpperTriangular{Float64, Matrix{Float64}}:
 3.16228  0.316228
  ⋅       3.14643

julia> cholesky(A) # or do this
Cholesky{Float64, Matrix{Float64}}
U factor:
2×2 UpperTriangular{Float64, Matrix{Float64}}:
 3.16228  0.316228
  ⋅       3.14643

i.e. "Even if the tag is stripped off, the underlying matrix must still be symmetric", which is needlessly strong.

[ViralBShah]

The new wording just reads a bit strange. Perhaps it can be updated?

[WalterMadelim]

Well, I think the existing docstring itself is a bit inconsistent.

SparseArrays.jl/src/solvers/cholmod.jl

Lines 1538 to 1545 in b225a85

	"""
	cholesky(A::SparseMatrixCSC; shift = 0.0, check = true, perm = nothing) -> CHOLMOD.Factor
	Compute the Cholesky factorization of a sparse positive definite matrix `A`.
	`A` must be a [`SparseMatrixCSC`](@ref) or a [`Symmetric`](@ref)/[`Hermitian`](@ref)
	view of a `SparseMatrixCSC`. Note that even if `A` doesn't
	have the type tag, it must still be symmetric or Hermitian.
	If `perm` is not given, a fill-reducing permutation is used.

Observing line 1539, the arg A is restricted to be a SparseMatrixCSC. But in line 1542 it suggests A can be a Symmetric view.
But essentially, were A a view, it will not be dispatched to this method, strictly speaking, (am I right?).

julia> A::SparseMatrixCSC
3×3 SparseMatrixCSC{Float64, Int64} with 7 stored entries:
 10.0        0.519584   0.169799
  0.519584  10.0         ⋅
  0.169799    ⋅        10.0

julia> tagged_A = Symmetric(A)
3×3 Symmetric{Float64, SparseMatrixCSC{Float64, Int64}}:
 10.0        0.519584   0.169799
  0.519584  10.0         ⋅
  0.169799    ⋅        10.0

julia> tagged_A::SparseMatrixCSC
ERROR: TypeError: in typeassert, expected SparseMatrixCSC, got a value of type Symmetric{Float64, SparseMatrixCSC{Float64, Int64}}
Stacktrace:
 [1] top-level scope
   @ REPL[28]:1

A larger issue concerns the top-level design of LinearAlgebra.
I think LinearAlgebra and SparseArrays are not 2 separate objects. From the standpoint of a user, he should feel some sort of consistency. A matrix can have numerous "advantageous" structures---Symmetric, Banded, Sparse, etc.
From a text book sense, a Banded (e.g. Tridiagonal) matrix is surely also a Sparse matrix.
But the TriDiagonal belongs to LinearAlgebra.jl whereas SparseMatrixCSC is in SparseArrays.jl.

In LinearAlgebra.jl, we even have SymTriDiagonal, which is both Symmetric and TriDiagonal (which can be readily recognized).
But in this package, it seems we have to use a Symmetric view, as the tagged_A above shows. Therefore tagged_A::SparseMatrixCSC will ERROR.

Here are some of my thoughts (far from constructive). I'm still learning these two packages (LinearAlgebra and SparseArray). Because it's not purely about julia, it invoke 3rd party APIs. Maybe I need more time to have a deeper understanding.🙂

[ViralBShah]

My point is actually much simpler - that this sentence does not read as correct English - the use of itself.

Note that if `A` doesn't have the type tag, itself must be symmetric or Hermitian.

Maybe something like:

Note that if `A` doesn't have the type tag, it must itself be symmetric or Hermitian.

[ViralBShah]

IIRC, we simply ignore the lower triangular part if not actually symmetric - but need to look at the code for that. In that case the documentation should be updated accordingly.