Merge pull request #503 from JuliaDiff/ox/onlyfast

copysign and sincospi are not fastmath-able

Author: oxinabox

Project.toml

@@ -1,6 +1,6 @@
 name = "ChainRules"
 uuid = "082447d4-558c-5d27-93f4-14fc19e9eca2"
-version = "1.8.0"
+version = "1.8.1"
 
 [deps]
 ChainRulesCore = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4"

src/rulesets/Base/base.jl

@@ -1,6 +1,8 @@
 # See also fastmath_able.jl for where rules are defined simple base functions
 # that also have FastMath versions.
 
+@scalar_rule copysign(y, x) (ifelse(signbit(x)!=signbit(y), -one(y), +one(y)), NoTangent())
+
 @scalar_rule one(x) zero(x)
 @scalar_rule zero(x) zero(x)
 @scalar_rule transpose(x) true
@@ -145,6 +147,11 @@ end
 
 @scalar_rule sinc(x) cosc(x)
 
+# the position of the minus sign below warrants the correct type for π  
+if VERSION ≥ v"1.6"
+    @scalar_rule sincospi(x) @setup((sinpix, cospix) = Ω) (π * cospix)  (π * (-sinpix))
+end
+
 @scalar_rule(
     clamp(x, low, high),
     @setup(

src/rulesets/Base/fastmath_able.jl

@@ -48,10 +48,6 @@ let
         # Trig-Multivariate
         @scalar_rule atan(y, x) @setup(u = x ^ 2 + y ^ 2) (x / u, -y / u)
         @scalar_rule sincos(x) @setup((sinx, cosx) = Ω) cosx -sinx
-        # the position of the minus sign below warrants the correct type for π  
-        if VERSION ≥ v"1.6"
-            @scalar_rule sincospi(x) @setup((sinpix, cospix) = Ω) (π * cospix)  (π * (-sinpix))
-        end
 
         # exponents
         @scalar_rule cbrt(x) inv(3 * Ω ^ 2)
@@ -184,8 +180,6 @@ let
         @scalar_rule max(x, y) @setup(gt = x > y) (gt, !gt)
         @scalar_rule min(x, y) @setup(gt = x > y) (!gt, gt)
 
-        @scalar_rule copysign(y, x) (ifelse(signbit(x)!=signbit(y), -one(y), +one(y)), NoTangent())
-
         # Unary functions
         @scalar_rule +x true
         @scalar_rule -x -1
@@ -233,6 +227,9 @@ let
     fast_ast = Base.FastMath.make_fastmath(fastable_ast)
 
     # Guard against mistakenly defining something as fast-able when it isn't.
+    # NOTE: this check is not infallible, it will be tricked if a function itself is not
+    # fastmath_able but it's pullback uses something that is. So manual check should also be
+    # done.
     non_transformed_definitions = intersect(fastable_ast.args, fast_ast.args)
     filter!(expr->!(expr isa LineNumberNode), non_transformed_definitions)
     if !isempty(non_transformed_definitions)

test/rulesets/Base/base.jl

@@ -1,4 +1,14 @@
 @testset "base" begin
+    @testset "copysign" begin
+        # don't go too close to zero as the numerics may jump over it yielding wrong results
+        @testset "at $y" for y in (-1.1, 0.1, 100.0)  
+            @testset "at $x" for x in (-1.1, -0.1, 33.0)
+                test_frule(copysign, y, x)
+                test_rrule(copysign, y, x)
+            end
+        end
+    end
+    
     @testset "Trig" begin
         @testset "Basics" for x = (Float64(π)-0.01, Complex(π, π/2))
             test_scalar(sec, x)
@@ -47,6 +57,15 @@
         @testset "sinc" for x = (0.0, 0.434, Complex(0.434, 0.25))
             test_scalar(sinc, x)
         end
+
+        if VERSION ≥ v"1.6"
+            @testset "sincospi" for T in (Float64, ComplexF64)
+                Δz = Tangent{Tuple{T,T}}(randn(T), randn(T))
+
+                test_frule(sincospi, randn(T))
+                test_rrule(sincospi, randn(T); output_tangent=Δz)
+            end
+        end
     end  # Trig
 
     @testset "Angles" begin

test/rulesets/Base/fastmath_able.jl

@@ -64,14 +64,6 @@ const FASTABLE_AST = quote
                 test_frule(sincos, randn(T))
                 test_rrule(sincos, randn(T); output_tangent=Δz)
             end
-            if VERSION ≥ v"1.6"
-                @testset "sincospi(x::$T)" for T in (Float64, ComplexF64)
-                    Δz = Tangent{Tuple{T,T}}(randn(T), randn(T))
-
-                    test_frule(sincospi, randn(T))
-                    test_rrule(sincospi, randn(T); output_tangent=Δz)
-                end
-            end
         end
     end
 
@@ -192,16 +184,6 @@ const FASTABLE_AST = quote
         end
     end
 
-    @testset "copysign" begin
-        # don't go too close to zero as the numerics may jump over it yielding wrong results
-        @testset "at $y" for y in (-1.1, 0.1, 100.0)  
-            @testset "at $x" for x in (-1.1, -0.1, 33.0)
-                test_frule(copysign, y, x)
-                test_rrule(copysign, y, x)
-            end
-        end
-    end
-
     @testset "sign" begin
         @testset "real" begin
             @testset "at $x" for x in (-1.1, -1.1, 0.5, 100.0)