remove copy-allocation on accessing cholesky factors (.L, .U)

[st--]

(Port of JuliaLang/julia#42920)

In master, accessing fields L or U of Cholesky or CholeskyPivoted calls copy() on the transpose. Here I add tests thayt check that the accessors don't allocate more memory than is needed for e.g. the Adjoint() construction, and remove the unnecessary copy(). Generally julia seems to be trying hard to avoid unnecessary allocations, so I would consider this to be a bugfix, not a breaking change.

There is a related discourse thread: https://discourse.julialang.org/t/implementation-for-accessing-cholesky-factors/56219

Side note 1: there is also a copy() in the definition of _chol!(A::AbstractMatrix, ::Type{UpperTriangular}) - it's a bit less clear to me in what cases this will get run and whether it's okay to remove the copy(), so let me know if you think it'd be worth spending the time to go deeper into that also.

Side note 2: some of the code tests uplo using Cuplo === char_uplo(d), some of it using sym_uplo(Cuplo) == d, is there any particular reason for this, or would it be worth unifying it?

[stevengj]

Right now this leads to slightly slower triangular solves:

julia> using BenchmarkTools, LinearAlgebra

julia> A = rand(1000,1000); b = rand(1000); x = copy(b);

julia> U1 = LowerTriangular(A)';

julia> U2 = copy(U1);

julia> @btime ldiv!($x, $U1, $b);
  81.000 μs (0 allocations: 0 bytes)

julia> @btime ldiv!($x, $U2, $b);
  69.875 μs (0 allocations: 0 bytes)

although it still might be worth it to save allocations. (Presumably the ldiv! performance can be fixed by an optimized method if needed.)

Not sure what other code might be affected by this … in general, it seems okay since (a) the documentation does not specify whether a copy is made and (b) it is still returning a subtype of UpperTriangular or LowerTriangular.

[ViralBShah]

Test failures need to be fixed.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.89%. Comparing base (443aa0f) to head (cb2e5c7).
Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1186   +/-   ##
=======================================
  Coverage   91.89%   91.89%           
=======================================
  Files          34       34           
  Lines       15360    15360           
=======================================
  Hits        14115    14115           
  Misses       1245     1245           

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

[dkarrasch]

Right now this leads to slightly slower triangular solves:

This is just a matter of memory access, I think. The solves all run LAPACK's trtrs!.

[dkarrasch]

... or BLAS: #1194.