New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Special case x^0 #331
Special case x^0 #331
Conversation
@@ -455,4 +455,14 @@ for N in (0,3), M in (0,4), V in (Int, Float32) | |||
end | |||
end | |||
|
|||
@testset "Exponentiation of zero" begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought to mix this with the rest of the test but then can't think of a good way to turn this into a random test. So I created a separated testset. I also noticed that @testset
is not used anywhere but I assumed that's only because ForwardDiff.jl predates @testset
. I hope using @testset
is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that's fine.
else | ||
new_partials = partials(x) * y * ($f)(v, y - 1) | ||
end | ||
return Dual{Tx}(expv, new_partials) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I think the main concern here is performance/SIMDability.
A cleaner way to write this that might be easier on the compiler is
new_partials = ifelse(iszero(y), zero(partials(x)), partials(x) * y * ($f)(v, y - 1))
It'd be worth benchmarking this/looking at generated LLVM code to ensure there aren't any substantial regressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll do some benchmarks.
I guess we can also define Base.literal_pow
to do the branching at the compile time for some extra boost. Though it only works for the case exponent is an integer literal.
So I tried some benchmarks to compare this branch and the master. For example: using ForwardDiff: Dual
using BenchmarkTools
const SUITE = BenchmarkGroup()
function log1mx_taylor(p, x)
y = -x
@simd for n in 2:p
@inbounds y -= x^n / n
end
return y
end
for x in Dual.(-0.1:0.1:0.1, 1.0)
SUITE["log(1-$x)"] = @benchmarkable log1mx_taylor(1000, $x)
end
Link to the full output: https://gist.github.com/tkf/03e0b87c37db85b60bad91fef0a3efcb/967886d655b2268a9a3978700a02cd5f1e3acd15#file-bench_log1mx_taylor_judge-md The gist includes another benchmark ( I looked at diff of In the other case ( So, does it mean it's good to go? The fact that |
So I keep playing with the benchmark and I couldn't find clear difference. I wonder if this PR eliminates any possibility for SIMD utilization that was previously possible. First of all, we preserve "trivial" SIMD-ability. For example, with using ForwardDiff: Dual
a = Dual(1:1.0:5...)
@code_llvm a^2 does print So, I think the loss of SIMD-ability has to be assessed for the possibility of SIMD across multiple using ForwardDiff: Dual
using StaticArrays
x = Dual(1.0, 2.0)
xs = SVector(x, x, x, x)
@inline pow2(x) = x^2
f(xs) = pow2.(xs)
@code_llvm f(xs) which does not print The only way that I could come up with to make the above function So, I think it means that we don't loose much with this PR. The benchmarks (in the previous post) with more complex code support this as well. What do you think? |
Awesome, thanks for the performance analysis here. Let's merge this and I'll go take a look at #332. |
In current released (and master; 3f62885) ForwardDiff.jl,
n+1
-th derivative ofx^n
atx = 0
isNaN
:This issue can be observed more directly in nested dual number. Namely, if I have "
n
-th order" dual number (is it the right word?) with the.value
component being 0, itsn - 1
-th power producesNaN
in the.partials
component:All I need to make it "work" is to special-case
Dual(0.0, ...) ^ 0
(x1^0
above). I'm not familiar with the whole ForwardDiff.jl codebase so I'm not entirely sure if this is the right "fix" (or what I see here is actually a bug). But I can come up with some rationale for why this PR could be the right way to go:x3^2 === x3 * x3
andx2^1 === x2
(note the triple equal===
for comparing.partials
) for nestedDual
didn't hold without this PR.x^y
atx = y = 0
is problematic if bothx
andy
are variables. However, ify = 0
is a non-Dual
constant, I suppose we don't have such problem since this case corresponds to taking the limity -> 0
first and then do another limit forx
to compute the derivative.What do you think?
fixes #330