remove superfluous _symmetric() calls

[st--]

Summary
Remove _symmetric() calls on user-provided matrices that should be pos-def (and hence symmetric) already, as discussed in #236 (comment)

Proposed changes
We use _symmetric() to ensure we can compute cholesky of matrices that should be posdef (mathematically) but might not be (numerically, due to small rounding errors in matmul & co). This is the right thing to do for intermediate matrices, but when applied to user input directly, this risks just hiding mistakes. I.e., if a user provides an arbitrary (non-pos-def) matrix, it would simply mask the mistake, happily compute an answer (which will be misleading), instead of crashing & giving the user an indication that there's a mistake somewhere.

What alternatives have you considered?
We could add a check to _symmetric that the return value is approximately the same as the input. We could also explicitly check that the user input is symmetric (or nearly so), or enforce it to be some kind of PDMat type.

Breaking changes
I would consider this a bugfix, the only thing that would break is already-broken usage.

[codecov]

Codecov Report

Merging #239 (91258c3) into master (89afecb) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #239   +/-   ##
=======================================
  Coverage   98.05%   98.05%           
=======================================
  Files          10       10           
  Lines         360      360           
=======================================
  Hits          353      353           
  Misses          7        7           

Impacted Files	Coverage Δ
src/sparse_approximations.jl	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89afecb...91258c3. Read the comment docs.

[devmotion]

Thanks for opening this PR!

I would consider this a bugfix, the only thing that would break is already-broken usage.

I disagree: It breaks working code that operates with matrices that mathematically are guaranteed to be symmetric and positive definite but are not due to numerical inaccuracies and floating point issues.

It is also a bit unfortunate that such numerical issues are not caught when FiniteGP is constructed. One alternative approach that is eg used in Distances and possibly soon in PDMats is to check positive semi-definiteness at construction and then assume internally that the matrix is positive semi-definite and eg wrap it in Symmetric if helpful for downstream methods:

struct MyType{T}
    A::T

    function MyType{T}(A::T; check_posdef::Bool=true) where {T}
         if check_posdef
             eigmin(A) >= 0 || error("A is not positive semi-definite")
         end
         return new{T}(A)
    end
end

eigmin is fast for ScalMat, Diagonal etc. and the check could also be disabled if desired. And internally we could use numerical optimizations such as _symmetric without making incorrect assumptions.

[willtebbutt]

This PR has gone stale, so I'm going to close. @st-- please do re-open if you want to push it forwards.