JuliaLang / LinearAlgebra.jl

Julia Linear Algebra standard library
Other
17 stars 4 forks source link

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

Closed IanButterworth closed 4 months ago

IanButterworth commented 7 months ago

Seen on this 1.10 backports PR https://github.com/JuliaLang/julia/pull/53714 but the problematic commit was reverted on that PR feceefe (#53714)

Quoting https://github.com/JuliaLang/julia/pull/53714#issuecomment-2052421573


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 https://github.com/JuliaLang/julia/blob/97b0571cbd74cfc7959fe065b5dfc47d0636fc4e/stdlib/LinearAlgebra/src/LinearAlgebra.jl#L241

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

IanButterworth commented 4 months ago

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 commented 4 months ago

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

aviatesk commented 4 months ago

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
IanButterworth commented 4 months ago

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