From cc8f0cd07b7ea05d06f60bc1fd485cd3b8aad40b Mon Sep 17 00:00:00 2001 From: Lyndon White Date: Tue, 27 Jul 2021 11:52:44 +0100 Subject: [PATCH 1/2] Don't try and convert to FloatX except if Integer or AbstractFloat --- Project.toml | 2 +- src/projection.jl | 37 ++++++++++++++++++++++++++++++++++--- test/projection.jl | 25 ++++++++++++++++++++++++- 3 files changed, 59 insertions(+), 5 deletions(-) diff --git a/Project.toml b/Project.toml index d6720e438..7791f2ca6 100644 --- a/Project.toml +++ b/Project.toml @@ -1,6 +1,6 @@ name = "ChainRulesCore" uuid = "d360d2e6-b24c-11e9-a2a3-2a2ae2dbcce4" -version = "1.0.1" +version = "1.0.2" [deps] Compat = "34da2185-b29b-5c13-b0c7-acf172513d20" diff --git a/src/projection.jl b/src/projection.jl index 7fb968fb9..f67af54af 100644 --- a/src/projection.jl +++ b/src/projection.jl @@ -135,13 +135,44 @@ ProjectTo(::Real) = ProjectTo{Real}() ProjectTo(::Complex) = ProjectTo{Complex}() ProjectTo(::Number) = ProjectTo{Number}() for T in (Float16, Float32, Float64, ComplexF16, ComplexF32, ComplexF64) - # Preserve low-precision floats as accidental promotion is a common perforance bug + # Preserve low-precision floats as accidental promotion is a common perforance bug @eval ProjectTo(::$T) = ProjectTo{$T}() end ProjectTo(x::Integer) = ProjectTo(float(x)) ProjectTo(x::Complex{<:Integer}) = ProjectTo(float(x)) -(::ProjectTo{T})(dx::Number) where {T<:Number} = convert(T, dx) -(::ProjectTo{T})(dx::Number) where {T<:Real} = convert(T, real(dx)) + +# Preserve low-precision floats as accidental promotion is a common perforance bug +(::ProjectTo{T})(dx::AbstractFloat) where T<:AbstractFloat = convert(T, dx) +(::ProjectTo{T})(dx::Integer) where T<:AbstractFloat = convert(T, dx) + + +# We asked for a number/real and they gave use one. We did ask for a particular concrete +# type, but that is just for the preserving low precision floats, which is handled above. +# Any Number/Real actually occupies the same subspace, so we can trust them. +# In particular, this makes weirder Real subtypes that are not simply the values like +# ForwardDiff.Dual and Symbolics.Sym work, because we stay out of their way. +(::ProjectTo{<:Number})(dx::Number) where {T<:Number} = dx +(::ProjectTo{<:Real})(dx::Real) = dx + +(::ProjectTo{T})(dx::Complex) where T<:Real = ProjectTo(zero(T))(real(dx)) + +# Complex +function (proj::ProjectTo{<:Complex{<:AbstractFloat}})( + dx::Complex{<:Union{AbstractFloat,Integer}} +) + # in this case we can just convert as we know we are dealing with + # boring floating point types or integers + return convert(project_type(proj), dx) +end +# Pass though non-AbstractFloat to project each component +function (::ProjectTo{<:Complex{T}})(dx::Complex) where T + project = ProjectTo(zero(T)) + return Complex(project(real(dx)), project(imag(dx))) +end +function (::ProjectTo{<:Complex{T}})(dx::Real) where T + project = ProjectTo(zero(T)) + return Complex(project(dx), project(zero(dx))) +end # Arrays # If we don't have a more specialized `ProjectTo` rule, we just assume that there is diff --git a/test/projection.jl b/test/projection.jl index 209542781..f80c8f2c0 100644 --- a/test/projection.jl +++ b/test/projection.jl @@ -2,6 +2,15 @@ using ChainRulesCore, Test using LinearAlgebra, SparseArrays using OffsetArrays, BenchmarkTools +# Like ForwardDiff.jl's Dual +struct Dual{T<:Real} <: Real + value::T + partial::T +end +Base.real(x::Dual) = x +Base.float(x::Dual) = Dual(float(x.value), float(x.partial)) +Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) + @testset "projection" begin ##### @@ -12,14 +21,28 @@ using OffsetArrays, BenchmarkTools # real / complex @test ProjectTo(1.0)(2.0 + 3im) === 2.0 @test ProjectTo(1.0 + 2.0im)(3.0) === 3.0 + 0.0im + @test ProjectTo(2.0+3.0im)(1+1im) === 1.0+1.0im + @test ProjectTo(2.0)(1+1im) === 1.0 + # storage - @test ProjectTo(1)(pi) === Float64(pi) + @test ProjectTo(1)(pi) === pi @test ProjectTo(1 + im)(pi) === ComplexF64(pi) @test ProjectTo(1//2)(3//4) === 3//4 @test ProjectTo(1.0f0)(1 / 2) === 0.5f0 @test ProjectTo(1.0f0 + 2im)(3) === 3.0f0 + 0im @test ProjectTo(big(1.0))(2) === 2 + @test ProjectTo(1.0)(2) === 2.0 + end + + @testset "Dual" begin # some weird Real subtype that we should basically leave alone + @test ProjectTo(1.0)(Dual(1.0, 2.0)) isa Dual + @test ProjectTo(1.0)(Dual(1, 2)) isa Dual + @test ProjectTo(1.0 + 1im)(Dual(1.0, 2.0)) isa Complex{<:Dual} + @test ProjectTo(1.0 + 1im)( + Complex(Dual(1.0, 2.0), Dual(1.0, 2.0)) + ) isa Complex{<:Dual} + @test ProjectTo(1.0)(Complex(Dual(1.0, 2.0), Dual(1.0, 2.0))) isa Dual end @testset "Base: arrays of numbers" begin From 325480d8c19d09a07f128e52fa79746d4558f28b Mon Sep 17 00:00:00 2001 From: Lyndon White Date: Tue, 27 Jul 2021 13:15:21 +0100 Subject: [PATCH 2/2] Do not project integers into floats Apply suggestions from code review Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com> remove redundant real --- src/projection.jl | 40 +++++++++++----------------------------- test/projection.jl | 37 +++++++++++++++++++------------------ 2 files changed, 30 insertions(+), 47 deletions(-) diff --git a/src/projection.jl b/src/projection.jl index f67af54af..fa9ebe38b 100644 --- a/src/projection.jl +++ b/src/projection.jl @@ -103,9 +103,6 @@ julia> unthunk(pd(th)) 1.0 ⋅ ⋅ ⋅ 5.0 ⋅ ⋅ ⋅ 9.0 - -julia> ProjectTo([1 2; 3 4]') # no special structure, integers are promoted to float(x) -ProjectTo{AbstractArray}(element = ProjectTo{Float64}(), axes = (Base.OneTo(2), Base.OneTo(2))) ``` """ ProjectTo(::Any) # just to attach docstring @@ -135,15 +132,15 @@ ProjectTo(::Real) = ProjectTo{Real}() ProjectTo(::Complex) = ProjectTo{Complex}() ProjectTo(::Number) = ProjectTo{Number}() for T in (Float16, Float32, Float64, ComplexF16, ComplexF32, ComplexF64) - # Preserve low-precision floats as accidental promotion is a common perforance bug + # Preserve low-precision floats as accidental promotion is a common performance bug @eval ProjectTo(::$T) = ProjectTo{$T}() end -ProjectTo(x::Integer) = ProjectTo(float(x)) -ProjectTo(x::Complex{<:Integer}) = ProjectTo(float(x)) -# Preserve low-precision floats as accidental promotion is a common perforance bug +# Preserve low-precision floats as accidental promotion is a common performance bug (::ProjectTo{T})(dx::AbstractFloat) where T<:AbstractFloat = convert(T, dx) -(::ProjectTo{T})(dx::Integer) where T<:AbstractFloat = convert(T, dx) +# In this case we can just `convert` as we know we are dealing with boring `AbstractFloat`s +(::ProjectTo{T})(dx::Complex{<:AbstractFloat}) where {T<:Complex{<:AbstractFloat}} = convert(T, dx) +(::ProjectTo{T})(dx::AbstractFloat) where {T<:Complex{<:AbstractFloat}} = convert(T, dx) # We asked for a number/real and they gave use one. We did ask for a particular concrete @@ -151,28 +148,13 @@ ProjectTo(x::Complex{<:Integer}) = ProjectTo(float(x)) # Any Number/Real actually occupies the same subspace, so we can trust them. # In particular, this makes weirder Real subtypes that are not simply the values like # ForwardDiff.Dual and Symbolics.Sym work, because we stay out of their way. -(::ProjectTo{<:Number})(dx::Number) where {T<:Number} = dx -(::ProjectTo{<:Real})(dx::Real) = dx +(::ProjectTo{<:Number})(dx::Number) = dx +# If you remove the above julia sometimes can't find the (::ProjectTo{T})(::T) for complex T + +(project::ProjectTo{<:Real})(dx::Complex) = project(real(dx)) +(::ProjectTo{<:Complex})(dx::Real) = Complex(dx) # Complex is overloaded it insert zero imag -(::ProjectTo{T})(dx::Complex) where T<:Real = ProjectTo(zero(T))(real(dx)) -# Complex -function (proj::ProjectTo{<:Complex{<:AbstractFloat}})( - dx::Complex{<:Union{AbstractFloat,Integer}} -) - # in this case we can just convert as we know we are dealing with - # boring floating point types or integers - return convert(project_type(proj), dx) -end -# Pass though non-AbstractFloat to project each component -function (::ProjectTo{<:Complex{T}})(dx::Complex) where T - project = ProjectTo(zero(T)) - return Complex(project(real(dx)), project(imag(dx))) -end -function (::ProjectTo{<:Complex{T}})(dx::Real) where T - project = ProjectTo(zero(T)) - return Complex(project(dx), project(zero(dx))) -end # Arrays # If we don't have a more specialized `ProjectTo` rule, we just assume that there is @@ -372,7 +354,7 @@ ProjectTo(x::SymTridiagonal{T}) where {T<:Number} = generic_projector(x) function (project::ProjectTo{SymTridiagonal})(dx::AbstractMatrix) dv = project.dv(diag(dx)) ev = project.ev((diag(dx, 1) .+ diag(dx, -1)) ./ 2) - return SymTridiagonal(dv, ev) + return SymTridiagonal(promote(dv, ev)...) end (project::ProjectTo{SymTridiagonal})(dx::SymTridiagonal) = generic_projection(project, dx) diff --git a/test/projection.jl b/test/projection.jl index f80c8f2c0..03d3f9009 100644 --- a/test/projection.jl +++ b/test/projection.jl @@ -21,18 +21,19 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) # real / complex @test ProjectTo(1.0)(2.0 + 3im) === 2.0 @test ProjectTo(1.0 + 2.0im)(3.0) === 3.0 + 0.0im - @test ProjectTo(2.0+3.0im)(1+1im) === 1.0+1.0im - @test ProjectTo(2.0)(1+1im) === 1.0 + @test ProjectTo(2.0+3.0im)(1+1im) === 1 + 1im + @test ProjectTo(2.0)(1+1im) === 1 # storage @test ProjectTo(1)(pi) === pi - @test ProjectTo(1 + im)(pi) === ComplexF64(pi) + @test ProjectTo(1 + im)(pi) === Complex(pi) @test ProjectTo(1//2)(3//4) === 3//4 @test ProjectTo(1.0f0)(1 / 2) === 0.5f0 - @test ProjectTo(1.0f0 + 2im)(3) === 3.0f0 + 0im + @test ProjectTo(1.0f0 + 2im)(3) === 3 + 0im + @test ProjectTo(1.0f0 + 2im)(3.0) === 3f0 + 0f0im @test ProjectTo(big(1.0))(2) === 2 - @test ProjectTo(1.0)(2) === 2.0 + @test ProjectTo(1.0)(2) === 2 end @testset "Dual" begin # some weird Real subtype that we should basically leave alone @@ -48,9 +49,9 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) @testset "Base: arrays of numbers" begin pvec3 = ProjectTo([1, 2, 3]) @test pvec3(1.0:3.0) === 1.0:3.0 - @test pvec3(1:3) == 1.0:3.0 # would prefer ===, map(Float64, dx) would do that, not important + @test pvec3(1:3) == 1:3 @test pvec3([1, 2, 3 + 4im]) == 1:3 - @test eltype(pvec3([1, 2, 3.0f0])) === Float64 + @test eltype(pvec3([1, 2, 3.0f0])) === Float32 # reshape @test pvec3(reshape([1, 2, 3], 3, 1)) isa Vector @@ -59,7 +60,7 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) pmat = ProjectTo(rand(2, 2) .+ im) @test pmat([1 2; 3 4.0+5im]') isa Adjoint # pass-through - @test pmat([1 2; 3 4]') isa Matrix{ComplexF64} # broadcast type change + @test pmat([1 2; 3 4]') isa Matrix{<:Complex} # broadcast type change pmat2 = ProjectTo(rand(2, 2)') @test pmat2([1 2; 3 4.0+5im]) isa Matrix # adjoint matrices are not re-created @@ -87,7 +88,7 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) @test_throws MethodError ProjectTo(Any[:x, :y]) @test ProjectTo(Any[1, 2])(1:2) == [1.0, 2.0] # projects each number. - @test Tuple(ProjectTo(Any[1, 2 + 3im])(1:2)) === (1.0, 2.0 + 0.0im) + @test Tuple(ProjectTo(Any[1, 2 + 3im])(1:2)) === (1, 2 + 0im) @test ProjectTo(Any[true, false]) isa ProjectTo{NoTangent} # empty arrays @@ -108,14 +109,14 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) @test pzed isa ProjectTo{AbstractArray} pref = ProjectTo(Ref(2.0)) - @test pref(Ref(3 + im))[] === 3.0 - @test pref(4)[] === 4.0 # also re-wraps scalars + @test pref(Ref(3 + im))[] === 3 + @test pref(4)[] === 4 # also re-wraps scalars @test pref(Ref{Any}(5.0)) isa Base.RefValue{Float64} pref2 = ProjectTo(Ref{Any}(6 + 7im)) - @test pref2(Ref(8))[] === 8.0 + 0.0im + @test pref2(Ref(8))[] === 8 + 0im prefvec = ProjectTo(Ref([1, 2, 3 + 4im])) # recurses into contents - @test prefvec(Ref(1:3)) isa Base.RefValue{Vector{ComplexF64}} + @test prefvec(Ref(1:3)) isa Base.RefValue{<:Vector{<:Complex}} @test_throws DimensionMismatch prefvec(Ref{Any}(1:5)) end @@ -126,7 +127,7 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) @testset "LinearAlgebra: $adj vectors" for adj in [transpose, adjoint] # adjoint vectors padj = ProjectTo(adj([1, 2, 3])) - adjT = typeof(adj([1, 2, 3.0])) + adjT = adj == transpose ? Transpose : Adjoint @test padj(transpose(1:3)) isa adjT @test padj([4 5 6 + 7im]) isa adjT @test padj([4.0 5.0 6.0]) isa adjT @@ -182,15 +183,15 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) @test ProjectTo(UpperTriangular(randn(3, 3) .> 0))(randn(3, 3)) == NoTangent() # an experiment with allowing subspaces which aren't subtypes - @test psymm(Diagonal([1, 2, 3])) isa Diagonal{Float64} - @test pupp(Diagonal([1, 2, 3 + 4im])) isa Diagonal{Float64} + @test psymm(Diagonal([1, 2, 3])) isa Diagonal{Int} + @test pupp(Diagonal([1, 2, 3 + 4im])) isa Diagonal{Int} end @testset "LinearAlgebra: sparse structured matrices" begin pdiag = ProjectTo(Diagonal(1:3)) @test pdiag(reshape(1:9, 3, 3)) == Diagonal([1, 5, 9]) @test pdiag(pdiag(reshape(1:9, 3, 3))) == pdiag(reshape(1:9, 3, 3)) - @test pdiag(rand(ComplexF32, 3, 3)) isa Diagonal{Float64} + @test pdiag(rand(ComplexF32, 3, 3)) isa Diagonal{Float32} @test pdiag(Diagonal(1.0:3.0)) === Diagonal(1.0:3.0) @test ProjectTo(Diagonal(randn(3) .> 0))(randn(3, 3)) == NoTangent() @test ProjectTo(Diagonal(randn(3) .> 0))(Diagonal(rand(3))) == NoTangent() @@ -295,7 +296,7 @@ Base.zero(x::Dual) = Dual(zero(x.value), zero(x.partial)) th = @thunk 1 + 2 + 3 pth = ProjectTo(4 + 5im)(th) @test pth isa Thunk - @test unthunk(pth) === 6.0 + 0.0im + @test unthunk(pth) === 6 + 0im end @testset "display" begin