Slightly loosen a test so that linalg tests pass when using MKL #42315
Conversation
| @@ -152,7 +152,7 @@ end | |||
| for vf in (copy(vvf), view(vvf, 1:3)), C in (copy(CC), view(CC, 1:3, 1:3)) | |||
| @test mul!(C, vf, transpose(vf)) == vf*vf' | |||
| C .= C0 = rand(eltype(C), size(C)) | |||
| @test mul!(C, vf, transpose(vf), 2, 3) == 2vf*vf' .+ 3C0 | |||
| @test mul!(C, vf, transpose(vf), 2, 3) ≈ 2vf*vf' .+ 3C0 | |||
There was a problem hiding this comment.
I'm wondering if this was exact equality for a reason, cc @tkf
There was a problem hiding this comment.
The difference is 1e-15 in one of the elements. 3 of these tests fail with MKL.
There was a problem hiding this comment.
I think I simply copied the pattern from mul!(C, vf, transpose(vf)) == vf*vf' just above.
There was a problem hiding this comment.
In the test above, I think the idea is that it should be the exact same code path for the right and left hand side and therefore exact equality. It wasn't obvious to me that this test should take the same path for right and left hand side but wondered if that was your intention. If not then this PR should be fine.
There was a problem hiding this comment.
Yeah, sorry, I was sloppy. This change LGTM too.
|
I wonder if there's a way that we could keep this test as exact equality by default, and only switch to approximate equality if the backend is MKL. |
|
Isn't approximate equality what you would expect? |
I was surprised to see exact equality in that test. I would have expected it to be approximate.
I don't think it is worthwhile. We'll want to add support for Accelerate too (see the M1 issues) and what not. It would be much nicer to have a test suite that passes on all BLAS. The tolerances are quite stringent. |
|
Marking as backport for both 1.6 and 1.7, so that MKL tests could in theory pass on both. |
(cherry picked from commit 4f3a89e)
(cherry picked from commit 4f3a89e)
(cherry picked from commit 4f3a89e)
(cherry picked from commit 4f3a89e)
Fixes JuliaLinearAlgebra/MKL.jl#88
The MKL.jl tests have now been updated to run the entire LinearAlgebra test suite. Thus, when PkgEval runs its tests, it will be nice to see that MKL.jl tests fully pass. In order for that to happen, we want to have this PR merged and included in 1.7 as well (in the next RC).