Skip to content
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

Fix DiffRules-based definitions for complex-valued functions #577

Merged
merged 4 commits into from
Apr 27, 2022

Conversation

devmotion
Copy link
Member

Currently, complex-valued functions (with real inputs!) for which derivatives are defined in DiffRules don't work with ForwardDiff since the return values for DiffRules-based definitions is enforced to be a Dual but should be a Complex{<:Dual} in these cases. E.g., the following example fails currently (and is therefore excluded from the tests):

julia> using ForwardDiff, SpecialFunctions

julia> hankelh1(1, 2.0)
0.5767248077568733 - 0.10703243154093756im

julia> hankelh1(1, ForwardDiff.Dual(2.0))
ERROR: ArgumentError: Cannot create a dual over scalar type Any. If the type behaves as a scalar, define ForwardDiff.can_dual(::Type{Any}) = true.
Stacktrace:
 [1] throw_cannot_dual(V::Type)
   @ ForwardDiff ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:41
 [2] ForwardDiff.Dual{Nothing, Any, 1}(value::ComplexF64, partials::ForwardDiff.Partials{1, Any})
   @ ForwardDiff ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:18
 [3] Dual
   @ ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:60 [inlined]
 [4] Dual
   @ ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:64 [inlined]
 [5] Dual
   @ ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:67 [inlined]
 [6] Dual
   @ ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:71 [inlined]
 [7] hankelh1(x::Int64, y::ForwardDiff.Dual{Nothing, Float64, 0})
   @ ForwardDiff ~/.julia/packages/ForwardDiff/PBzup/src/dual.jl:249
 [8] top-level scope
   @ REPL[9]:1

This PR fixes this issue and returns values of type Complex{<:Dual} if the primal function value is a Complex number. This fixes the example above:

julia> using ForwardDiff, SpecialFunctions

julia> hankelh1(1, 2.0)
0.5767248077568733 - 0.10703243154093756im

julia> hankelh1(1, ForwardDiff.Dual(2.0))
Dual{Nothing}(0.5767248077568733) - Dual{Nothing}(0.10703243154093756)*im

@devmotion
Copy link
Member Author

Bump 🙂

@devmotion
Copy link
Member Author

Bump 🙂 It would be very helpful, in DiffRules and downstream packages, to fix this bug.

@devmotion devmotion requested a review from mcabbott April 24, 2022 01:44
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022

Codecov Report

Merging #577 (1043a59) into master (e11936c) will decrease coverage by 0.62%.
The diff coverage is 65.21%.

@@            Coverage Diff             @@
##           master     #577      +/-   ##
==========================================
- Coverage   85.43%   84.80%   -0.63%     
==========================================
  Files           9        9              
  Lines         865      882      +17     
==========================================
+ Hits          739      748       +9     
- Misses        126      134       +8     
Impacted Files Coverage Δ
src/dual.jl 73.76% <65.21%> (-1.16%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e11936c...1043a59. Read the comment docs.

@andreasnoack
Copy link
Member

There have been a couple of discussions of support for complex numbers. Might be good to address the issues here.

@devmotion
Copy link
Member Author

devmotion commented Apr 26, 2022

Might be good to address the issues here.

Which issues do you have in mind?

Complex numbers (in intermediate calculations!) are already supported, e.g.,

julia> ForwardDiff.gradient([1.0, 2.0]) do x
           abs(log(x[1] + im * x[2]))
       end
2-element Vector{Float64}:
 -0.20597271407900614
  0.39695748015993854

julia> (x -> log(x[1] + im*x[2]))([ForwardDiff.Dual(1.0), ForwardDiff.Dual(2.0)])
Dual{Nothing}(0.8047189562170501) + Dual{Nothing}(1.1071487177940904)*im

This PR just fixes incorrect ForwardDiff-definitions of rules present in DiffRules but doesn't change the support for complex numbers in general. In particular the PR does not add support for complex numbers as inputs, it just makes it possible to use e.g. hankelh1 in a chain of calculations with real-valued inputs and outputs.

@andreasnoack
Copy link
Member

Okay. Then let's get this one in.

@devmotion devmotion merged commit 62d557b into master Apr 27, 2022
@devmotion devmotion deleted the dw/diffrules_complex branch April 27, 2022 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants