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

Test failure in LinearAlgebra/addmul #1092

Closed
giordano opened this issue Sep 15, 2024 · 2 comments · Fixed by JuliaLang/julia#56210
Closed

Test failure in LinearAlgebra/addmul #1092

giordano opened this issue Sep 15, 2024 · 2 comments · Fixed by JuliaLang/julia#56210
Labels
bug Something isn't working test This change adds or pertains to unit tests

Comments

@giordano
Copy link
Contributor

On 4633607ce9

The global RNG seed was 0xadd355ff29e71246580cec9b4759733c.

Error in testset LinearAlgebra/addmul:
Test Failed at ~/repo/julia/usr/share/julia/stdlib/v1.12/LinearAlgebra/test/addmul.jl:175
  Expression: ≈(collect(returned_vec), α * Ac * xc + β * yc, rtol = rtol)
   Evaluated: [0.00016286048013114396, 0.0] ≈ [0.0001627827388785974, 0.0] (rtol=0.0003452669770922512)

Offending test is https://github.com/JuliaLang/julia/blob/4633607ce9b9f077f32f89f09a136e04389bbac2/stdlib/LinearAlgebra/test/addmul.jl#L169-L175 I could reliably reproduce this with Base.runtests("LinearAlgebra/addmul"; seed=0xadd355ff29e71246580cec9b4759733c) on both x86_64-linux and aarch64-linux. Note: this is not a test reintroduced by JuliaLang/julia#55775

@giordano giordano added bug Something isn't working test This change adds or pertains to unit tests labels Sep 15, 2024
@jishnub
Copy link
Collaborator

jishnub commented Sep 15, 2024

Here is a simpler and a more concrete reproducer extracted from the testsets:

julia> using LinearAlgebra

julia> A = Diagonal(Float32[1.2805837, -0.83571815]);

julia> xc = [-10, 0];

julia> yc = [4.479540055608876, 0.0];

julia> α = -0.32607044710547134; β = -0.9321140763694151;

julia> y1 = α * A * xc + β * yc
2-element Vector{Float64}:
 0.0001627827388785974
 0.0

julia> y2 = mul!(copy(yc), A, xc, α, β)
2-element Vector{Float64}:
 0.00016286048013114396
 0.0

julia> norm(y1 - y2)/norm(y1)
0.000477576757106528

@jishnub
Copy link
Collaborator

jishnub commented Sep 16, 2024

This seems to be an issue with associativity.
The operation A * xc * α is computed as

mat_vec_scalar(A, x, γ) = A * (x * γ)

whereas the in-place multiplication operates elementwise, and performs the first multiplication before the second one.

julia> (A * xc * α)[1]
4.175605124232544

julia> A * xc * α == A * (xc * α)
true

julia> ((A * xc) * α)[1]
4.175605201973797

julia> ((A * xc) * α)[1] == A[1,1] * xc[1] * α
true

This is exacerbated by the fact that A * xc * α is very close in magnitude and opposite in sign to yc * β, so the rounding error has a large impact on the rtol to which the results match.

julia> (A * xc * α)[1], (yc * β)[1]
(4.175605124232544, -4.175442341493666)

julia> y1 = (A * xc * α)[1] + (yc * β)[1]
0.0001627827388785974

julia> y2 = ((A * xc) * α)[1] + (yc * β)[1]
0.00016286048013114396

julia> norm(y1 -y2)/norm(y1)
0.000477576757106528

Since we are comparing small numbers, perhaps we need an atol here.

KristofferC referenced this issue Nov 14, 2024
This avoids the issues as in
https://github.com/JuliaLang/julia/issues/55781 and
https://github.com/JuliaLang/julia/issues/55779 where we compare small
numbers using a relative tolerance. Also, in this PR, I have added an
extra test, so now we compare both `A * B * alpha + C * beta` and `A * B
* alpha - C * beta` with the corresponding in-place versions. The idea
is that if the terms `A * B * alpha` and ` C * beta` have similar
magnitudes, at least one of the two expressions will usually result in a
large enough number that may be compared using a relative tolerance.

I am unsure if the `atol` chosen here is optimal, as I have ballparked
it to use the maximum `eps` by looking at all the `eltype`s involved.

Fixes #55781
Fixes #55779
@KristofferC KristofferC transferred this issue from JuliaLang/julia Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants