Limit Dual to Real primals, replace all Number with Real

[gdalle]

Fix #91 for good

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 74.19355% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 79.62%. Comparing base (f1bf915) to head (46c9879).
Report is 1 commits behind head on main.

Files	Patch %	Lines
src/overload_dual.jl	0.00%	4 Missing ⚠️
src/conversion.jl	87.50%	1 Missing ⚠️
src/overload_connectivity.jl	50.00%	1 Missing ⚠️
src/pattern.jl	66.66%	1 Missing ⚠️
src/tracers.jl	85.71%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #95   +/-   ##
=======================================
  Coverage   79.62%   79.62%           
=======================================
  Files          17       17           
  Lines         702      702           
=======================================
  Hits          559      559           
  Misses        143      143           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[adrhill]

I'm not sure we actually need the other changes besides the one I commented on. Would be interested in discussing this.

[adrhill]

This is all we need.

[gdalle]

I thought so too. But then I ran the tests and found that leaving the ::Number dispatches led to some method ambiguities, typically between

<=(::Real, ::Real)
<=(::SCT.Dual, ::Number)

[adrhill]

I would even argue that it would be more informative to have MethodErrors due to type restrictions occur here rather than in Base.promote_rule.

[gdalle]

See the discussion below, we shouldn't promise complex number support anyway. Tracers are real numbers, and if people want to go beyond they can do Complex{Tracer}

[adrhill]

The original motivation for this PR is that #92 introduced a weird edge case:

Some weirdness resulting from [#92]: our local tracer type Dual can contain a Complex primal value but is Real.
Maybe the most sane fix is to constrain the primal to Real as well.

This PR however removes a lot of functionality from ConnectivityTracer and other global tracers without any obvious gain. For example, the following doesn't work anymore:

connectivity_pattern(x -> x^(2im), 1)

[gdalle]

This PR however removes a lot of functionality from ConnectivityTracer and other global tracers without any obvious gain.

Point taken for ConnectivityTracer. As for the other tracers, I must disagree.
We don't know how to autodiff with complex numbers, or it's very complicated (see this Discourse thread). So claiming that we can properly trace first and second derivatives for complex numbers is a bit of an oversell, which this PR remedies.

If we wanted to promise that a complex derivative is zero, derivative would that be anyway? The holomorphic derivative $\mathbb{C} \to \mathbb{C}$, or the Jacobian of the function taken as a mapping $\mathbb{R}^2 \to \mathbb{R}^2$?

[gdalle]

However, I note that ForwardDiff doesn't constrain the primal and partial values inside their Dual struct, only the Dual itself must subtype Real:

https://github.com/JuliaDiff/ForwardDiff.jl/blob/1907196ba50e3eb337860301ece3ec37d0582257/src/dual.jl#L14-L21

Still, based on what I understand from previous discussion, the right way to proceed is to make a Complex{Dual}, not a Dual{Complex}.

Related:

• Support for complex numbers JuliaDiff/ForwardDiff.jl#157 (in particular this comment) closed by Relax complex function signatures to make them ForwardDiff compatible JuliaLang/julia#36030
• Vote: should we add Complex number support as a feature of ForwardDiff. JuliaDiff/ForwardDiff.jl#456

[gdalle]

I think however that we should be able to support functions that do $\mathbb{R} \to \mathbb{C} \to \mathbb{R}$ out of the box, so the last commit reintroduces the former complex-valued tests by projecting them to the real axis

[adrhill]

the right way to proceed is to make a Complex{Dual}, not a Dual{Complex}.

This I like. I guess this requires adding some new Base.promote_rule?

[gdalle]

This I like. I guess this requires adding some new Base.promote_rule?

The test I added back failed and that's probably the reason, will investigate

[gdalle]

Here's an argument for only supporting real inputs (nevermind the output type):

• we established in Make AbstractTracer a subtype of Real, not Number #91 that many functions in the ecosystem are limited to ::AbstractArray{<:Real}
• a Dual{Complex} <: Real doesn't make much sense to me

[adrhill]

Here's an argument for only supporting real inputs (nevermind the output type):

• we established in Make AbstractTracer a subtype of Real, not Number #91 that many functions in the ecosystem are limited to ::AbstractArray{<:Real}
• a Dual{Complex} <: Real doesn't make much sense to me

I agree with all of this. My suggestion was that a Complex{Float64} input could be converted to

• Complex{T} for non-dual tracers, T<:Union{ConnectivityTracer, GradientTracer, HessianTracer}.
• Complex{Dual{Float64, T}} for dual tracers.

[adrhill]

This would require additional trace_input and create_tracer methods for Complex numbers.

[adrhill]

But all of this is not something that has to be done in this PR (or has to be done in general until the need for it arises).