Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement to_vec for matrix factorizations #128

Merged
merged 5 commits into from
Jul 29, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/to_vec.jl
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,24 @@ function to_vec(x::Cholesky)
return x_vec, Cholesky_from_vec
end

function to_vec(x::S) where {S <: Union{LinearAlgebra.QRCompactWYQ, LinearAlgebra.QRCompactWY}}
x_vec, back = to_vec([x.factors, x.T])
function to_vec(x::S) where {U, S <: Union{LinearAlgebra.QRCompactWYQ{U}, LinearAlgebra.QRCompactWY{U}}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this is valid for QRCompactWYQ? If it is, it should at least be tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QRCompactWYQ is represented with the same T and factors matrices than QRCompactWY, yes. I added the tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks!

# x.T is composed of upper triangular blocks. The subdiagonals elements
# of the blocks are abitrary. We make sure to set all of them to zero
# to avoid NaN.
blocksize, cols = size(x.T)
T = zeros(U, blocksize, cols)

for i in 0:div(cols - 1, blocksize)
used_cols = i * blocksize
n = min(blocksize, cols - used_cols)
T[1:n, (1:n) .+ used_cols] = UpperTriangular(view(x.T, 1:n, (1:n) .+ used_cols))
end

x_vec, back = to_vec([x.factors, T])

function QRCompact_from_vec(v)
factors, T = back(v)
return S(factors, T)
factors, Tback = back(v)
return S(factors, Tback)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that using S like this here will not be type stable. Since it's not super performance critical, it might not be worth all the trouble though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why is that so?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end
return x_vec, QRCompact_from_vec
end
Expand Down
22 changes: 19 additions & 3 deletions test/to_vec.jl
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ function test_to_vec(x::T; check_inferred=true) where {T}
x_vec, back = to_vec(x)
@test x_vec isa Vector
@test all(s -> s isa Real, x_vec)
@test all(!isnan, x_vec)
check_inferred && @inferred back(x_vec)
@test x == back(x_vec)
return nothing
Expand Down Expand Up @@ -117,12 +118,27 @@ end
end

@testset "Factorizations" begin
for dims in [(5, 5), (4, 6), (7, 3)]
# (100, 100) is needed to test for the NaNs that can appear in the
# qr(M).T matrix
for dims in [(5, 5), (4, 6), (7, 3), (100, 100)]
Kolaru marked this conversation as resolved.
Show resolved Hide resolved
M = randn(T, dims...)
P = M * M' + I # Positive definite matrix
test_to_vec(svd(M); check_inferred = true)
test_to_vec(qr(M))
test_to_vec(svd(M))
test_to_vec(cholesky(P))

# Special treatment for QR since it is represented by a matrix
# with some arbirtrary values.
F = qr(M)
@inferred to_vec(F)
F_vec, back = to_vec(F)
@test F_vec isa Vector
@test all(s -> s isa Real, F_vec)
@test all(!isnan, F_vec)
@inferred back(F_vec)
F_back = back(F_vec)
@test F.Q == F.Q
@test F.R == F.R
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this test? Would be good to also add the to_vec(qr(A)) == to_vec(qr(A)) test I proposed above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an oversight from me, it should read F_back.Q == F.Q, to test that back(to_vec(qr(A))) correctly gives back the input.

Your suggested test was on the next line, I have reformulated it to make more explicit what is tested.

@test F_vec == first(to_vec(F))
end
end

Expand Down