Closed anriseth closed 8 years ago
line 30-34 should be removed from operator.jl
Feng
On Tue, Dec 1, 2015 at 1:22 PM, Asbjørn Nilsen Riseth < notifications@github.com> wrote:
In operator.jl:30 you define a differentiate function for SymbolParameter{:abs2}.
Have I missed something, or is this the correct definition of the derivative of abs2? It overwrites the definition used in Calculus.jl, and gives the wrong derivative rule (should be :(2x))
julia> function differentiate(x::SymbolParameter{:abs2},args,wrt) x = args[1] xp = differentiate(x,wrt) return :($x>0?$xp:-$xp) end WARNING: Method definition differentiate(Calculus.SymbolParameter{:abs2}, Any, Any) in module Calculus at /home/riseth/.julia/v0.4/Calculus/src/differentiate.jl:206 overwritten in module Main at none:2. differentiate (generic function with 85 methods)
julia> differentiate("abs2(x)", [:x])1-element Array{Any,1}: :(if x > 0 1 else -1 end)
I put an inline comment on the commit that added this function. I'm not sure if they give anyone a notification, so I created an issue as well.
— Reply to this email directly or view it on GitHub https://github.com/fqiang/ReverseDiffTape.jl/issues/3.
Thank @anriseth. Please verify the fix.
I think it should be something like this instead?
function differentiate(x::SymbolParameter{:abs2},args,wrt) x = args[1] xp = differentiate(:(x^2),wrt) return :($x>0?$xp:-$xp) end
@fqiang, that seems wrong as well. For a Real x, I believe abs2(x) = x*x
, so you shouldn't look at the case x>0
at all.
Is there a reason why you don't want to use the version of differentiate that exists in Calculus.jl already, e.g. because of speed issues? If not, it seems to me more sensible just to remove the whole function overwrite.
@anriseth I think you are right.
In
operator.jl:30
you define adifferentiate
function for SymbolParameter{:abs2}.Have I missed something, or is this the correct definition of the derivative of abs2? It overwrites the definition used in Calculus.jl, and gives the wrong derivative rule (should be
:(2x)
)I put an inline comment on the commit that added this function. I'm not sure if they give anyone a notification, so I created an issue as well.