eliminate uses of full from base/linalg/lq.jl

[Sacha0]

Another small step towards deprecation of full. Ref. JuliaLang/LinearAlgebra.jl#229, JuliaLang/LinearAlgebra.jl#231, #18850, and linked threads. Best!

[andreasnoack]

I'm also wondering if requesting the full matrix from LQPackedQ is a sufficiently common operation to require a separate function.

[andreasnoack]

This is not square. Should probably be eye(eltype(Q), size(Q.factors, 2)).

[Sacha0]

Good catch! I failed to read the existing code critically. And agreed, eye(eltype(Q), size(Q.factors, 2)) looks correct. What was the object the existing code returns?

[andreasnoack]

It looks like this PR is turning into a significant bug fix. The current version gives

julia> full(lqfact(randn(4,2))[:Q], thin = true)
2×2 Array{Float64,2}:
 -0.759173  0.650889
  0.650889  0.759173

julia> full(lqfact(randn(4,2))[:Q], thin = false)
2×4 Array{Float64,2}:
 -0.652378  -0.757894  0.0  0.0
 -0.757894   0.652378  0.0  0.0

julia> full(lqfact(randn(2,4))[:Q], thin = false)
4×2 Array{Float64,2}:
 -0.22708   -0.390319
  0.72372   -0.204318
 -0.025446   0.888878
 -0.651166  -0.125704

julia> full(lqfact(randn(2,4))[:Q], thin = false)
4×2 Array{Float64,2}:
 -0.0604746   0.111308
  0.822683   -0.406272
 -0.158067   -0.81894
  0.542725    0.389731

so only the first one is correct. Here is how they are supposed to look like

[Sacha0]

I'm also wondering if requesting the full matrix from LQPackedQ is a sufficiently common operation to require a separate function.

To clarify, are you suggesting squareQ(Q::LQPackedQ) should not exist, or something beyond squareQ(Q::LQPackedQ) should exist? Thanks!

[andreasnoack]

To clarify, are you suggesting squareQ(Q::LQPackedQ) should not exist

Yes. I'm suggesting that it shouldn't exist. I think it is pretty rare that you actually want to do that. If you, for instructional purposes, want to work with the full Q you would probably call qr instead of qrfact in the first place. We could also mention in the documentation that the dense Q can be computed from a package Q via multiplication with eye. It is very simple and also instructive since it would be how we'd compute Q anyway.

[Sacha0]

Yes. I'm suggesting that it shouldn't exist. I think it is pretty rare that you actually want to do that. If you, for instructional purposes, want to work with the full Q you would probably call qr instead of qrfact in the first place. We could also mention in the documentation that the dense Q can be computed from a package Q via multiplication with eye. It is very simple and also instructive since it would be how we'd compute Q anyway.

Sounds good on this end :).

[Sacha0]

Revised. Thoughts? Thanks! :)

[kshyatt]

@andreasnoack as the author of that function, I included it for consistency with qr.jl but have no particular attachment to it otherwise.

[Sacha0]

Thanks all! :)