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

guard against LAPACK least squares solver test failure, round 2 #25418

Merged
merged 1 commit into from
Jan 6, 2018

Conversation

Sacha0
Copy link
Member

@Sacha0 Sacha0 commented Jan 5, 2018

#25287 modified a LAPACK least squares solver test to guard against failure due to poor conditioning of the test matrices. Unfortunately that modification proved insufficient as I ran into the motivating failure again locally

julia> srand(0xab6820ee372333e0a07585ed1a906b41);

julia> @testset "gelsy, gelsd" begin
           let elty = ComplexF64
               A = rand(elty, 10, 10)
               B = rand(elty, 10, 10)
               C, j = LAPACK.gelsd!(copy(A),copy(B))
               D, k = LAPACK.gelsy!(copy(A),copy(B))
               @test C ≈ D rtol=eps(cond(A))
           end
       end
gelsy, gelsd: Test Failed at REPL[34]:7
  Expression: ≈(C, D, rtol=eps(cond(A)))
   Evaluated: Complex{Float64}[-0.0262218+0.441148im 0.0346061-0.375153im … -0.0593423+0.645078im -0.166242+0.71145im; 0.392516+0.673879im 0.727514+0.518957im … 0.864918-0.22036im 1.0831+0.158312im; … ; 0.283946-0.078362im -0.161999-0.0885647im … -0.173693+0.390815im -0.474193-0.293889im; 0.41677-0.422742im 1.48165+0.0538742im … -0.153264-0.742447im 0.636379-0.666813im] ≈ Complex{Float64}[-0.0262218+0.441148im 0.0346061-0.375153im … -0.0593423+0.645078im -0.166242+0.71145im; 0.392516+0.673879im 0.727514+0.518957im … 0.864918-0.22036im 1.0831+0.158312im; … ; 0.283946-0.078362im -0.161999-0.0885647im … -0.173693+0.390815im -0.474193-0.293889im; 0.41677-0.422742im 1.48165+0.0538742im … -0.153264-0.742447im 0.636379-0.666813im] (rtol=3.552713678800501e-15)
Stacktrace:
 [1] macro expansion at ./REPL[34]:7 [inlined]
 [2] macro expansion at /Users/sacha/pkg/julia/usr/share/julia/site/v0.7/Test/src/Test.jl:980 [inlined]
 [3] top-level scope at ./REPL[34]:2
Test Summary: | Fail  Total
gelsy, gelsd  |    1      1
ERROR: Some tests did not pass: 0 passed, 1 failed, 0 errored, 0 broken.

This pull request relaxes the test tolerance just enough for the test to pass 100,000 consecutive repetitions:

julia> srand(0xab6820ee372333e0a07585ed1a906b41);

julia> @testset "gelsy, gelsd" begin
           @testset for elty in (Float32, Float64, ComplexF32, ComplexF64)
               for k in 1:10^5
                   A = rand(elty, 10, 10)
                   B = rand(elty, 10, 10)
                   C, j = LAPACK.gelsd!(copy(A),copy(B))
                   D, k = LAPACK.gelsy!(copy(A),copy(B))
                   @test C ≈ D rtol=4*eps(cond(A))
               end
           end
       end
Test Summary: |   Pass   Total
gelsy, gelsd  | 400000  400000
Test.DefaultTestSet("gelsy, gelsd", Any[Test.DefaultTestSet("elty = Float32", Any[], 100000, false), Test.DefaultTestSet("elty = Float64", Any[], 100000, false), Test.DefaultTestSet("elty = Complex{Float32}", Any[], 100000, false), Test.DefaultTestSet("elty = Complex{Float64}", Any[], 100000, false)], 0, false)

(Factors of 2 and 3 rather than 4 were insufficient.) Best!

@Sacha0 Sacha0 added linear algebra Linear algebra test This change adds or pertains to unit tests labels Jan 5, 2018
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 6, 2018

(AV timeout seems unrelated.)

@Sacha0
Copy link
Member Author

Sacha0 commented Jan 6, 2018

CI being happy, Fredrik having approved, Andreas having suggested the approach on slack, and this being a trivial change, absent objections or requests for time I plan to merge this pull request tomorrow midday PT at the earliest. Best!

@fredrikekre fredrikekre merged commit 08620e5 into JuliaLang:master Jan 6, 2018
@Sacha0 Sacha0 deleted the gelsy branch January 6, 2018 18:22
@Sacha0
Copy link
Member Author

Sacha0 commented Jan 6, 2018

Thanks all! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants