reflect change in rotate! from LinearAlgebra.jl

[tam724]

This reflects a change in LinearAlgebra.jl (JuliaLang/LinearAlgebra.jl#1323). The previous implementation of rotate! did not respect false as a strong zero. For consistency, the implementation in GPUArrays.jl should also be adapted.

Essentially, this covers the edge case:

julia> using GPUArrays, JLArrays

julia> v1 = JLArray([NaN, NaN]);
julia> v2 = JLArray([NaN, NaN]);

julia> GPUArrays.rotate!(v1, v2, false, false) # before PR
([0.0, 0.0], [NaN, NaN])

julia> GPUArrays.rotate!(v1, v2, false, false) # with PR
([0.0, 0.0], [0.0, 0.0])

• Should NaN / false behaviour be tested against?
• Should this wait until the new LinearAlgebra.jl version is released?

[github-actions]

Your PR requires formatting changes to meet the project's style guidelines.
Please consider running Runic (git runic master) to apply these changes.

Click here to view the suggested changes.

diff --git a/src/host/linalg.jl b/src/host/linalg.jl
index 4933839..a21d694 100644
--- a/src/host/linalg.jl
+++ b/src/host/linalg.jl
@@ -687,8 +687,8 @@ function LinearAlgebra.rotate!(x::AbstractGPUArray, y::AbstractGPUArray, c::Numb
         i = @index(Global, Linear)
         @inbounds xi = x[i]
         @inbounds yi = y[i]
-        @inbounds x[i] = s*yi +      c *xi
-        @inbounds y[i] = c*yi - conj(s)*xi 
+        @inbounds x[i] = s * yi + c * xi
+        @inbounds y[i] = c * yi - conj(s) * xi
     end
     rotate_kernel!(get_backend(x))(x, y, c, s; ndrange = size(x))
     return x, y
diff --git a/test/testsuite/linalg.jl b/test/testsuite/linalg.jl
index 433f762..3660dca 100644
--- a/test/testsuite/linalg.jl
+++ b/test/testsuite/linalg.jl
@@ -303,7 +303,7 @@
 
     @testset "rotate!" for T in eltypes
         @test compare(rotate!, AT, rand(T,5), rand(T,5), Ref(rand(real(T))), Ref(rand(T)))
-        if isfloattype(T) && false # skip because the LinAlg.jl change is not released 
+        if isfloattype(T) && false # skip because the LinAlg.jl change is not released
             @test compare(rotate!, AT, fill(T(NaN), 5), fill(T(NaN), 5), Ref(false), Ref(false))
         end
     end
@@ -345,7 +345,7 @@ end
         @test compare(mul!, AT, y, f(A), x)
         @test compare(mul!, AT, y, f(A), x, Ref(T(4)), Ref(T(5)))
         #TODO: generic_matvecmul! (from LinearAlgebra.jl) does not respect the "strong zero" for Float16
-        if isfloattype(T) && !(T==Float16) && !(T == ComplexF16)
+        if isfloattype(T) && !(T == Float16) && !(T == ComplexF16)
             y_NaN, A_NaN, x_NaN = fill(T(NaN), 4), fill(T(NaN), 4, 4), fill(T(NaN), 4)
             @test compare(mul!, AT, y_NaN, f(A_NaN), x_NaN, Ref(false), Ref(false))
         end

[maleadt]

Thanks! Mind adding those examples as a test?

[tam724]

Not sure if this is the right approach. Should we also compare "strong zero"/NaN behaviour against LinearAlgebra.jl or should this be guaranteed only by GPUArrays.jl ? In other words: should the tests use compare or should we test for !any(isnan()) in the output arrays?

For consistency I added similar tests for lmul, rmul, axp{b}y, rotate, reflect and mul.
To have them passing locally I had to exclude rotate (is fixed in LinearAlgebra.jl#master) and mul for Float16 (another inconsistency in LinAlg).

Essentially this now tests whether the multiplication with false stops the propagation of NaNs, if any NaNs would remain in the output array in GPUArrays and LinearAlgebra, the tests would fail, because (NaN ≈ NaN) gives false. If we would like to compare the "NaN treatment", we would have to compare with isequal (see: https://docs.julialang.org/en/v1/base/numbers/#Base.NaN)

[maleadt]

Ideally we test compatibility against Array/stdlibs, but if that behavior is clearly buggy it's fine to include VERSION checks to gate tests. So the most complete solution would be to use isnan on older versions, compare on newer. I'd normally suggest to only to the latter, but since the new behaviour isn't part of any Julia version, we should at least have some tests checking isnan.