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

Fix for #48911: Bugfix for the generic left-divide (ldiv!) algorithm with pivoted QR decomposition and wide matrices #49570

Merged
merged 6 commits into from
May 5, 2023

Conversation

aravindh-krishnamoorthy
Copy link
Contributor

@aravindh-krishnamoorthy aravindh-krishnamoorthy commented Apr 29, 2023

Fix for JuliaLang/LinearAlgebra.jl#991 + rebase to 219dc10

Summary:

  1. Fix the application of the elementary transformation (correct complex conjugation) to the solution as per LAPACK zlarzb https://netlib.org/lapack/explore-html/d5/ddd/zlarzb_8f_source.html
  2. Fix a complex conjugate bug in the original solution for Solution of overdetermined linear systems in Complex Bigfloat erroneous LinearAlgebra.jl#991, align with LAPACK zlarzb, https://netlib.org/lapack/explore-html/d5/ddd/zlarzb_8f_source.html
  3. Fix output permutation when A.P is not the identity matrix. (This aspect was removed during integration as the permutation is taken care by ldiv!. Previously the code was tested in isolation.)
  4. Align the final permutation with LAPACK's ZGELSY at https://netlib.org/lapack/explore-html/d8/d6e/zgelsy_8f_source.html (This aspect was removed during integration)
  5. Improve permutation performance (This aspect was removed during integration)

1. Fix output permutation when A.P is not the identity matrix.
2. Fix the application of the elementary transformation (correct complex conjugation) to the solution as per LAPACK zlarzb https://netlib.org/lapack/explore-html/d5/ddd/zlarzb_8f_source.html
3. Align the final permutation with LAPACK's ZGELSY at https://netlib.org/lapack/explore-html/d8/d6e/zgelsy_8f_source.html
4. Fix a complex conjugate bug in the original solution for #48911, align with LAPACK zlarzb, https://netlib.org/lapack/explore-html/d5/ddd/zlarzb_8f_source.html
5. Improve permutation performance
@giordano
Copy link
Contributor

giordano commented Apr 29, 2023

@aravindh-krishnamoorthy can you please edit the title of the PR to make it more descriptive without exclusively referencing an issue number? Also, linking a pull request to an issue works only in the commit message, not the title.

EDIT: sorry for closing the PR, I fat-fingered the button while writing the message, it wasn't even complete yet.

@giordano giordano closed this Apr 29, 2023
@giordano giordano reopened this Apr 29, 2023
@aravindh-krishnamoorthy aravindh-krishnamoorthy changed the title Fix for #48911 Fix for #48911: Bugfix for the generic left-divide (ldiv!) algorithm with pivoted QR decomposition and wide matrices Apr 29, 2023
@aravindh-krishnamoorthy
Copy link
Contributor Author

@giordano Thanks! Just edited it.

@N5N3 N5N3 added linear algebra Linear algebra needs tests Unit tests are required for this change labels Apr 30, 2023
@aravindh-krishnamoorthy
Copy link
Contributor Author

@N5N3 Is anything further required from my side for this PR?

@N5N3
Copy link
Member

N5N3 commented May 2, 2023

You just need to add an MWE of JuliaLang/LinearAlgebra.jl#991 in stdlib/LinearAlgebra/test/qr.jl.

@aravindh-krishnamoorthy
Copy link
Contributor Author

@N5N3 The tests are included. Thanks! The tests helped me fix a bug. The new test is called "issue JuliaLang/LinearAlgebra.jl#991" and is located at the end. Here's the report (run in the LinearAlgebra/test directory):

julia> using Revise
julia> using LinearAlgebra
julia> Revise.track(LinearAlgebra)
julia> include("qr.jl")
Test Summary:   | Pass  Total   Time
eltya = Float32 |  740    740  12.1s
Test Summary:   | Pass  Total  Time
eltya = Float64 |  740    740  4.7s
Test Summary:      | Pass  Total  Time
eltya = ComplexF32 |  740    740  5.5s
Test Summary:      | Pass  Total  Time
eltya = ComplexF64 |  740    740  4.1s
Test Summary:    | Pass  Total   Time
eltya = BigFloat |  740    740  10.8s
Test Summary: | Pass  Total  Time
eltya = Int64 |  500    500  1.8s
Test Summary:    | Pass  Total  Time
transpose errors |    3      3  0.2s
Test Summary: | Pass  Total  Time
Issue 7304    |    1      1  0.0s
Test Summary:        | Pass  Total  Time
qr on AbstractVector |    8      8  0.4s
Test Summary: | Pass  Total  Time
QR on Ints    |    1      1  0.2s
Test Summary: | Pass  Total  Time
Issue 16520   |    1      1  0.1s
Test Summary: | Pass  Total  Time
Issue 22810   |    2      2  0.1s
Test Summary: | Pass  Total  Time
Issue 24107   |    1      1  0.2s
Test Summary:                               | Pass  Total  Time
Issue 24589. Promotion of rational matrices |    1      1  0.1s
Test Summary:                                               | Pass  Total  Time
Issue Test Factorization fallbacks for rectangular problems |    8      8  0.1s
Test Summary:                         | Pass  Total  Time
Issue reflector of zero-length vector |    3      3  0.2s
Test Summary:                          | Pass  Total  Time
det(Q::Union{QRCompactWYQ, QRPackedQ}) |  376    376  0.5s
Test Summary:    | Pass  Total  Time
inv(::AbstractQ) |    4      4  0.0s
Test Summary:         | Pass  Total  Time
QR factorization of Q |    8      8  0.1s
Test Summary:                     | Pass  Total  Time
Generation of orthogonal matrices |    2      2  0.2s
Test Summary:                           | Pass  Total  Time
Multiplication of Q by special matrices |   16     16  0.6s
Test Summary: | Pass  Total  Time
copyto! for Q |   12     12  1.9s
Test Summary: | Pass  Total  Time
adjoint of QR |   48     48  0.0s
Test Summary: | Pass  Total  Time
issue JuliaLang/LinearAlgebra.jl#801  |    2      2  0.1s
Test Summary:           | Pass  Total  Time
convert between eltypes |    6      6  1.0s
Test Summary:                       | Pass  Total  Time
optimized getindex for an AbstractQ |   79     79  0.7s
Test Summary: | Pass  Total  Time
issue JuliaLang/LinearAlgebra.jl#991  |    2      2  0.8s
Main.TestQR

@N5N3 N5N3 removed the needs tests Unit tests are required for this change label May 2, 2023
@dkarrasch dkarrasch added the bugfix This change fixes an existing bug label May 4, 2023
@dkarrasch
Copy link
Member

I'd suggest to expicitly including the original example as a test case, and test that the solution is close to [1 - 0.5im, 0.5 + 1im]. In fact, I believe this test might be sufficient to add, because it reveals all those fine mistakes we had earlier.

@dkarrasch
Copy link
Member

Also, could you please update the initial summary. There are listed many things, but in the end it's "just" a few ' here and there, and nothing related to permutations, for instance.

@aravindh-krishnamoorthy
Copy link
Contributor Author

aravindh-krishnamoorthy commented May 4, 2023

Also, could you please update the initial summary. There are listed many things, but in the end it's "just" a few ' here and there, and nothing related to permutations, for instance.

Sorry, the permutation thing was because of the isolated unit tests. I took @maleadt's suggestion and moved to Revise.jl completely now. And that should avoid such problems in the future.

@dkarrasch Done! Added the original one specified by @andreasvarga, and retained the other two tests as I felt they tested a few other aspects of the function.

@dkarrasch
Copy link
Member

While you're at it, could you please check the new test cases with all sorts of qr(A, SomePivotStrategy()) (i.e., instead of using \, call qr(A, ...) first and then solve with the factorization and not with A directly), to make sure that the complex generic case works for all pivoting strategies? If something comes up, that doesn't need to be fixed here, but should be reported as an issue.

@dkarrasch dkarrasch added the merge me PR is reviewed. Merge when all tests are passing label May 4, 2023
@KristofferC KristofferC merged commit e70354d into JuliaLang:master May 5, 2023
@aravindh-krishnamoorthy aravindh-krishnamoorthy deleted the 48911 branch May 5, 2023 07:28
@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants