Conversation
| function SpecialFunctions.logabsgamma(d::Dual{T,<:Real}) where {T} | ||
| x = value(d) | ||
| y, s = SpecialFunctions.logabsgamma(x) | ||
| return (Dual{T}(y, SpecialFunctions.digamma(x) * partials(d)), s) |
There was a problem hiding this comment.
Maybe this should return rather
| return (Dual{T}(y, SpecialFunctions.digamma(x) * partials(d)), s) | |
| return (Dual{T}(y, SpecialFunctions.digamma(x) * partials(d)), Dual{T}(s, zero(s))) |
to be consistent with sign(::Dual)?
There was a problem hiding this comment.
it doesn't really matter one way or the other does it? promotion will fix it when it matters.
There was a problem hiding this comment.
No, I don't think it matters. It felt easier for downstream computations and somewhat stronger to drop the partials completely. Only afterwards I noticed the inconsistency with sign and started to wonder if we should return a zero partial instead.
There was a problem hiding this comment.
Hmm.... Maybe we should be consistent with sign at all since it makes a difference in an example such as:
julia> using ForwardDiff
julia> f(x) = Inf * x
f (generic function with 1 method)
julia> g(x) = sign(x)
g (generic function with 1 method)
julia> g(x::ForwardDiff.Dual) = sign(ForwardDiff.value(x))
g (generic function with 2 methods)
julia> ForwardDiff.derivative(f ∘ g, 1.0)
0.0
julia> ForwardDiff.derivative(f ∘ sign, 1.0)
NaNThe "stronger" definition that drops the partial completely doesn't run into the NaN-safe mode issues and hence does not care about the non-finite result.
Maybe it's an argument for defining
Base.sign(x::Dual{<:Any,<:Real}) = sign(value(x))rather than changing this PR though.
Codecov Report
@@ Coverage Diff @@
## master #585 +/- ##
==========================================
+ Coverage 86.57% 86.63% +0.05%
==========================================
Files 9 9
Lines 894 898 +4
==========================================
+ Hits 774 778 +4
Misses 120 120
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I'm not a regular committer here.
But the math on this checks out,
and other than one comment on style, I think the tests look sufficient.
I would say give this a little time, then merge it if noone else raising objections.
Maybe 1 more day, since it has been open 7 days already.
Co-authored-by: Frames Catherine White <oxinabox@ucc.asn.au>
3 participants
This PR adds a definition of
SpecialFunctions.logabsgamma(::Dual). SpecialFunctions is a dependency of ForwardDiff already, and in constrast to most other functions in SpecialFunctions derivatives oflogabsgammacan't be defined via DiffRules sincelogabsgammais not a real-valued function but returns a tuple(log(abs(gamma(x)), sign(gamma(x))).