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: use instead of == for tr tests in symmetric.jl #55143

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

aviatesk
Copy link
Member

After investigating JuliaLang/LinearAlgebra.jl#1064, I found that the issue was not caused by the effects of checksquare, but by the use of the @simd macro within tr(::Matrix):

function tr(A::Matrix{T}) where T
n = checksquare(A)
t = zero(T)
@inbounds @simd for i in 1:n
t += A[i,i]
end
t
end

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 ==.

@aviatesk aviatesk added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Jul 16, 2024
@aviatesk aviatesk changed the title LinearAlgebra: use LinearAlgebra: use instead of == for tr tests in symmetric.jl Jul 16, 2024
…c.jl

After investigating JuliaLang/julia#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
Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, testing for equality only makes sense when you implicitly assume that the exact same code path is taken (and perhaps more).

@dkarrasch dkarrasch added the linear algebra Linear algebra label Jul 17, 2024
@aviatesk aviatesk merged commit 7c9a465 into master Jul 17, 2024
8 checks passed
@aviatesk aviatesk deleted the avi/54090 branch July 17, 2024 08:39
aviatesk added a commit that referenced this pull request Jul 17, 2024
…#55143)

After investigating JuliaLang/julia#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 pull request Jul 17, 2024
…#55143)

After investigating JuliaLang/julia#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 aviatesk removed backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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