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

LinearAlgebra: unstable return value, affected by lazy string & @show in error path #54090

Closed
IanButterworth opened this issue Apr 15, 2024 · 4 comments · Fixed by #55143
Closed
Assignees
Labels
compiler:simd instruction-level vectorization domain:complex Complex numbers domain:linear algebra Linear algebra existential crisis kind:bug Indicates an unexpected problem or unintended behavior kind:heisenbug This bug occurs unpredictably

Comments

@IanButterworth
Copy link
Sponsor Member

Seen on this 1.10 backports PR #53714 but the problematic commit was reverted on that PR feceefe (#53714)

Quoting #53714 (comment)


I confirmed locally that this error was introduced by feceefe (#53714)

tr: Test Failed at /Users/ian/Documents/GitHub/alts/julia/stdlib/LinearAlgebra/test/symmetric.jl:224
  Expression: tr(aherm) == tr(Hermitian(aherm))
   Evaluated: 2.6298493464368606 + 0.0im == 2.62984934643686

which is weird for just a change switching to lazy strings?

diff --git a/stdlib/LinearAlgebra/src/LinearAlgebra.jl b/stdlib/LinearAlgebra/src/LinearAlgebra.jl
index 494572b1e8dd8..7b3ff8f0db53d 100644
--- a/stdlib/LinearAlgebra/src/LinearAlgebra.jl
+++ b/stdlib/LinearAlgebra/src/LinearAlgebra.jl
@@ -238,14 +238,14 @@ julia> LinearAlgebra.checksquare(A, B)
 """
 function checksquare(A)
     m,n = size(A)
-    m == n || throw(DimensionMismatch("matrix is not square: dimensions are $(size(A))"))
+    m == n || throw(DimensionMismatch(lazy"matrix is not square: dimensions are $(size(A))"))
     m
 end
 
 function checksquare(A...)
     sizes = Int[]
     for a in A
-        size(a,1)==size(a,2) || throw(DimensionMismatch("matrix is not square: dimensions are $(size(a))"))
+        size(a,1)==size(a,2) || throw(DimensionMismatch(lazy"matrix is not square: dimensions are $(size(a))"))
         push!(sizes, size(a,1))
     end
     return sizes

Specifically, reverting just this one makes tests pass

m == n || throw(DimensionMismatch(lazy"matrix is not square: dimensions are $(size(A))"))

Also leaving it lazy and adding a @show m under that line makes test pass

@IanButterworth IanButterworth added kind:bug Indicates an unexpected problem or unintended behavior domain:linear algebra Linear algebra domain:complex Complex numbers labels Apr 15, 2024
@IanButterworth IanButterworth added kind:heisenbug This bug occurs unpredictably existential crisis labels Jul 11, 2024
@IanButterworth IanButterworth added the compiler:effects effect analysis label Jul 11, 2024
@IanButterworth
Copy link
Sponsor Member Author

Oscar here (grabbed Ian's phone)
@aviatesk can you look into this? It seems like we must have some horrible effects bug here (broken nothrow analysis possibly?)

@aviatesk
Copy link
Sponsor Member

Thanks for the ping. I'll look into it.

@aviatesk
Copy link
Sponsor Member

It appears that this error is caused by the @simd usage within tr(::Matrix{T}) where T rather than the effects modeling issues. It seems like using a lazy string in checksquare is not directly causing this error. This error happens non-deterministically, indicating that the issue is randomness caused by the macro.
MRE:

using LineaAlgebra, Test

for _ = 1:100; begin
    n = 10
    areal = randn(n,n)/2
    a = convert(Matrix{Float64}, areal)
    aherm = a' + a
    @test tr(aherm) == tr(Hermitian(aherm))

    aimg  = randn(n,n)/2
    areal = randn(n,n)/2
    a = convert(Matrix{ComplexF32}, complex.(areal, aimg))
    aherm = a' + a
    @test tr(aherm) == tr(Hermitian(aherm))
end; end

@aviatesk aviatesk added compiler:simd instruction-level vectorization and removed compiler:effects effect analysis labels Jul 16, 2024
aviatesk added a commit that referenced this issue Jul 16, 2024
After investigating #54090, I found that the issue was
not caused by the effects of `checksquare`, but by the use of the
`@simd` macro within `tr(::Matrix)`:
https://github.com/JuliaLang/julia/blob/0945b9d7740855c82a09fed42fbf6bc561e02c77/stdlib/LinearAlgebra/src/dense.jl#L373-L380

While simply removing the `@simd` macro was considered, the strict
left-to-right summation without `@simd` otherwise is not necessarily
more accurate, so I concluded that the problem lies in the test code,
which tests the (strict) equality of two different `tr` execution
results. I have modified the test code to use `≈` instead of `==`.

- fixes #54090
aviatesk added a commit that referenced this issue Jul 16, 2024
…c.jl

After investigating #54090, I found that the issue was
not caused by the effects of `checksquare`, but by the use of the
`@simd` macro within `tr(::Matrix)`:
https://github.com/JuliaLang/julia/blob/0945b9d7740855c82a09fed42fbf6bc561e02c77/stdlib/LinearAlgebra/src/dense.jl#L373-L380

While simply removing the `@simd` macro was considered, the strict
left-to-right summation without `@simd` otherwise is not necessarily
more accurate, so I concluded that the problem lies in the test code,
which tests the (strict) equality of two different `tr` execution
results. I have modified the test code to use `≈` instead of `==`.

- fixes #54090
@IanButterworth
Copy link
Sponsor Member Author

Thanks for investigating & fixing. I had thought this was a return type change from real to complex, hence the alarm.

@IanButterworth IanButterworth changed the title LinearAlgebra: unstable return type, affected by lazy string & @show in error path LinearAlgebra: unstable return value, affected by lazy string & @show in error path Jul 16, 2024
aviatesk added a commit that referenced this issue Jul 17, 2024
…#55143)

After investigating #54090, I found that the issue was
not caused by the effects of `checksquare`, but by the use of the
`@simd` macro within `tr(::Matrix)`:

https://github.com/JuliaLang/julia/blob/0945b9d7740855c82a09fed42fbf6bc561e02c77/stdlib/LinearAlgebra/src/dense.jl#L373-L380

While simply removing the `@simd` macro was considered, the strict
left-to-right summation without `@simd` otherwise is not necessarily
more accurate, so I concluded that the problem lies in the test code,
which tests the (strict) equality of two different `tr` execution
results. I have modified the test code to use `≈` instead of `==`.

- fixes #54090
aviatesk added a commit that referenced this issue Jul 17, 2024
…#55143)

After investigating #54090, I found that the issue was
not caused by the effects of `checksquare`, but by the use of the
`@simd` macro within `tr(::Matrix)`:

https://github.com/JuliaLang/julia/blob/0945b9d7740855c82a09fed42fbf6bc561e02c77/stdlib/LinearAlgebra/src/dense.jl#L373-L380

While simply removing the `@simd` macro was considered, the strict
left-to-right summation without `@simd` otherwise is not necessarily
more accurate, so I concluded that the problem lies in the test code,
which tests the (strict) equality of two different `tr` execution
results. I have modified the test code to use `≈` instead of `==`.

- fixes #54090
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:simd instruction-level vectorization domain:complex Complex numbers domain:linear algebra Linear algebra existential crisis kind:bug Indicates an unexpected problem or unintended behavior kind:heisenbug This bug occurs unpredictably
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants