Uh oh!
There was an error while loading. Please reload this page.
Remove unnecessary assignment to fix type instability in ForwardDiff.hessian! - #316
Merged
Conversation
KristofferC
commented
Apr 11, 2018
Collaborator
ForwardDiff.jl/src/gradient.jl Lines 69 to 73 in 4a62165 same thing? |
ContributorAuthor
It looks like the exact same problem can happen, but testing below, inference seems fine on 0.6 (for this toy example): julia>const S =randn(10,4) |> x -> x'* x;
julia>f(x) = x'* S * x /2
julia> x =randn(4);
julia>using ForwardDiff, Traceur
julia> hr = DiffResults.HessianResult(x);
julia> gcfg = ForwardDiff.GradientConfig(f, x);
julia> hcfg = ForwardDiff.HessianConfig(f, hr, x);
julia>@trace ForwardDiff.gradient!(hr, f, x, gcfg);
julia>@trace ForwardDiff.hessian!(hr, f, x, hcfg);
WARNING: Method definition hessian!(DiffResults.DiffResult{O, V, D} where D<:Tuplewhere V where O, Any, AbstractArray{T, N} where N where T) inmodule ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47 overwritten at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47.
WARNING: Method definition hessian!(DiffResults.DiffResult{O, V, D} where D<:Tuplewhere V where O, Any, AbstractArray{T, N} where N where T, ForwardDiff.HessianConfig{T, V, N, DG, DJ} where DJ where DG where N where V) inmodule ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47 overwritten at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47.
WARNING: Method definition hessian!(DiffResults.DiffResult{O, V, D} where D<:Tuplewhere V where O, Any, AbstractArray{T, N} where N where T, ForwardDiff.HessianConfig{T, V, N, DG, DJ} where DJ where DG where N where V, Base.Val{CHK}) inmodule ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47 overwritten at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47.
WARNING: Method definition extract_gradient!(Type{T}, DiffResults.DiffResult{O, V, D} where D<:Tuplewhere V where O, Real) inmodule ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:63 overwritten at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:63.
WARNING: Method definition extract_gradient!(Type{T}, DiffResults.DiffResult{O, V, D} where D<:Tuplewhere V where O, ForwardDiff.Dual{T, V, N} where N where V<:Realwhere T) inmodule ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:70 overwritten at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:70.
WARNING: Method definition vector_mode_gradient!(Any, F, Any, ForwardDiff.GradientConfig{T, V, N, D} where D where N where V) inmodule ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:102 overwritten at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:102.
(ForwardDiff.hessian!)(::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}, ::#f, ::Array{Float64,1}, ::ForwardDiff.HessianConfig{ForwardDiff.Tag{#f,Float64},Float64,10,Array{ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},Float64,10},10},1},Tuple{Array{ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},Float64,10},1},Array{ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},Float64,10},1}}}, ::Val{true}) at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47
result is assigned as DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}} at line 47
result is assigned as Core.Box at line 47
dynamic dispatch to (DiffResults.hessian)((Core.getfield)(_9::Core.Box, :contents)) at line 54
dynamic dispatch to (DiffResults.gradient)((Core.getfield)(_9::Core.Box, :contents)) at line 54
returns Any
(ForwardDiff.hessian!)(::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}, ::#f, ::Array{Float64,1}, ::ForwardDiff.HessianConfig{ForwardDiff.Tag{#f,Float64},Float64,10,Array{ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},Float64,10},10},1},Tuple{Array{ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},Float64,10},1},Array{ForwardDiff.Dual{ForwardDiff.Tag{#f,Float64},Float64,10},1}}}) at /home/celrod/.julia/v0.6/ForwardDiff/src/hessian.jl:47
returns Any
julia> ForwardDiff.chunksize(gcfg) ==length(x)
true
julia> ydual = ForwardDiff.vector_mode_dual_eval(f, x, gcfg);
julia>@which ForwardDiff.extract_gradient!(ForwardDiff.Tag{typeof(f),Float64}, hr, ydual)
extract_gradient!(::Type{T}, result::DiffResults.DiffResult, dual::ForwardDiff.Dual) where T in ForwardDiff at /home/celrod/.julia/v0.6/ForwardDiff/src/gradient.jl:70
julia>@trace ForwardDiff.extract_gradient!(ForwardDiff.Tag{typeof(f),Float64}, hr, ydual)
MutableDiffResult(14.576177213652889, ([-21.5326, -1.40267, -2.76614, -4.14998], [16.7264-0.1410161.73575.34906; -0.1410168.72789-1.63618-4.21548; 1.7357-1.636182.345481.39758; 5.34906-4.215481.397588.88728]))Perhaps things are simple enough to inference to succeed, but nesting gradients didn't cause a problem either: julia>g(x) =sum(abs2, DiffResults.gradient(ForwardDiff.gradient!(gr, f, x, gcfg3)))
g (generic function with 1 method)
julia> gcfg2 = ForwardDiff.GradientConfig(g, x);
julia>const gr = DiffResults.GradientResult(gcfg2.duals);
julia>const gcfg3 = ForwardDiff.GradientConfig(f, gcfg2.duals);
julia>@trace ForwardDiff.gradient!(hr, g, x, gcfg2)
MutableDiffResult(390.562342477784, ([556.839, -55.7623, -647.817, -61.8146], [11.2110.709601-9.07282-1.09975; 0.7096014.648051.76466-0.0891536; -9.072821.7646613.8650.435675; -1.09975-0.08915360.4356755.1435]))
julia>g2(x) =sum(abs2, DiffResults.gradient(ForwardDiff.gradient!(gr3, f, x, gcfg6)))
g2 (generic function with 1 method)
julia>h(x) =sum(abs2, DiffResults.gradient(ForwardDiff.gradient!(gr2, g2, x, gcfg5)))
h (generic function with 1 method)
julia> gcfg4 = ForwardDiff.GradientConfig(h, x);
julia>const gr2 = DiffResults.GradientResult(gcfg4.duals);
julia>const gcfg5 = ForwardDiff.GradientConfig(g2, gcfg4.duals);
julia>const gr3 = DiffResults.GradientResult(gcfg5.duals);
julia>const gcfg6 = ForwardDiff.GradientConfig(f, gcfg5.duals);
julia>@trace ForwardDiff.gradient!(hr, h, x, gcfg4)
MutableDiffResult(736667.2051329048, ([1.06113e6, -83656.7, -1.23535e6, -1.02606e5], [11.2110.709601-9.07282-1.09975; 0.7096014.648051.76466-0.0891536; -9.072821.7646613.8650.435675; -1.09975-0.08915360.4356755.1435]))
julia>@code_warntype ForwardDiff.gradient!(hr, h, x, gcfg4, Val(true))
Variables:#self# <optimized out>
result::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}
f::#h
x::Array{Float64,1}
cfg::ForwardDiff.GradientConfig{ForwardDiff.Tag{#h,Float64},Float64,4,Array{ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4},1}}#temp# <optimized out>
Body:begin goto 22:# line 34:
unless (4=== (Base.arraylen)(x::Array{Float64,1})::Int64)::Bool goto 8# line 35:$(Expr(:invoke, MethodInstance forvector_mode_gradient!(::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}, ::#h, ::Array{Float64,1}, ::ForwardDiff.GradientConfig{ForwardDiff.Tag{#h,Float64},Float64,4,Array{ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4},1}}), :(ForwardDiff.vector_mode_gradient!), :(result), :(f), :(x), :(cfg)))
goto 118:# line 37:$(Expr(:invoke, MethodInstance forchunk_mode_gradient!(::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}, ::#h, ::Array{Float64,1}, ::ForwardDiff.GradientConfig{ForwardDiff.Tag{#h,Float64},Float64,4,Array{ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4},1}}), :(ForwardDiff.chunk_mode_gradient!), :(result), :(f), :(x), :(cfg)))11:# line 39:return result::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}end::DiffResults.MutableDiffResult{2,Float64,Tuple{Array{Float64,1},Array{Float64,2}}}
julia>eltype(gcfg6)
ForwardDiff.Dual{ForwardDiff.Tag{#f,ForwardDiff.Dual{ForwardDiff.Tag{#g2,ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4}},ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4},4}},ForwardDiff.Dual{ForwardDiff.Tag{#g2,ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4}},ForwardDiff.Dual{ForwardDiff.Tag{#h,Float64},Float64,4},4},4}So it seems to be inferring correctly. We should either leave it as is, or to be safe, rename the assigned result every time: functionextract_gradient!(::Type{T}, result::DiffResult, dual::Dual) where {T}
result2 = DiffResults.value!(result, value(T, dual))
result3 = DiffResults.gradient!(result2, partials(T, dual))
return result3
end |
jrevels
commented
Apr 11, 2018
Member
The original example is happening because This is really a Julia compiler problem rather than a ForwardDiff problem, but if you care to put in the effort for a workaround here, the most straightforward strategy would probably be to manually instantiate a callable struct with the correct type parameters instead of using the closure. |
ContributorAuthor
I'll manually instantiate a struct. Will get to it soon. |
ContributorAuthor
I was considering a few more complicated routes to avoid allocating the EDIT: julia>@benchmark ForwardDiff.hessian!($hr2, f2, $max_density_params, $cfg2)
BenchmarkTools.Trial: memory estimate:5.39 KiB
allocs estimate:11--------------
minimum time:16.030 μs (0.00% GC)
median time:18.445 μs (0.00% GC)
mean time:18.849 μs (1.43% GC)
maximum time:1.451 ms (96.41% GC)
--------------
samples:10000
evals/sample:1following it: julia>@benchmark ForwardDiff.hessian!($hr2, f2, $max_density_params, $cfg2)
BenchmarkTools.Trial: memory estimate:5.39 KiB
allocs estimate:11--------------
minimum time:15.869 μs (0.00% GC)
median time:16.461 μs (0.00% GC)
mean time:17.133 μs (1.56% GC)
maximum time:1.396 ms (95.93% GC)
--------------
samples:10000
evals/sample:1....and 0.7: julia>@benchmark ForwardDiff.hessian!($hr2, f2, $max_density_params, $cfg2)
┌ Warning:`indmin` is deprecated, use `argmin` instead.
│ caller = minimum at trials.jl:112 [inlined]
└ @ Core trials.jl:112
┌ Warning:`indmax` is deprecated, use `argmax` instead.
│ caller = maximum at trials.jl:117 [inlined]
└ @ Core trials.jl:117
BenchmarkTools.Trial: memory estimate:768 bytes
allocs estimate:4--------------
minimum time:12.273 μs (0.00% GC)
median time:12.563 μs (0.00% GC)
mean time:12.751 μs (0.00% GC)
maximum time:60.073 μs (0.00% GC)
--------------
samples:10000
evals/sample:1 |
jrevels
commented
Jun 18, 2018
Member
Sorry for the late response, thanks for this! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before:
After:
The actual performance difference is negligible, although if the type instabilities "infected" other parts of your code, it could be a problem. Before:
after
These benchmarks were prior to merging the commits for specializing gradients on input functions, after which: