Add DiffResult for MArray

[charleskawczynski]

This PR adds

DiffResult(value::MArray, derivs::Tuple{Vararg{StaticArray}}) = MutableDiffResult(value, derivs)
DiffResult(value::SizedArray, derivs::Tuple{Vararg{StaticArray}}) = MutableDiffResult(value, derivs)

Fixes NLsolve.jl's 254. MArrays are mutable, so I think this needs to be defined differently than SArrays

Before this PR:

julia> using NLsolve
[ Info: Precompiling NLsolve [2774e3e8-f4cf-5e23-947b-6d7e65073b56]

julia> using StaticArrays

julia> function f!(F, x)
          F[1] = (x[1]+3)*(x[2]^3-7)+18
          F[2] = sin(x[2]*exp(x[1])-1)
       end
f! (generic function with 1 method)

julia> FT = Float32;

julia> a = MArray{Tuple{2},FT}(undef);

julia> a .= (0.1, 1.2);

julia> nlsolve(f!, a; autodiff = :forward)
ERROR: MethodError: no method matching extract_jacobian!(::Type{ForwardDiff.Tag{typeof(f!),Float32}}, ::DiffResults.ImmutableDiffResult{1,MArray{Tuple{2},Float32,1,2},Tuple{MArray{Tuple{2,2},Float32,2,4}}}, ::MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2}, ::Int64)
Closest candidates are:
  extract_jacobian!(::Type{T}, ::AbstractArray, ::AbstractArray, ::Any) where T at kawc\ForwardDiff.jl\src\jacobian.jl:112
  extract_jacobian!(::Type{T}, ::DiffResults.MutableDiffResult, ::AbstractArray, ::Any) where T at kawc\ForwardDiff.jl\src\jacobian.jl:120
Stacktrace:
 [1] vector_mode_jacobian!(::DiffResults.ImmutableDiffResult{1,MArray{Tuple{2},Float32,1,2},Tuple{MArray{Tuple{2,2},Float32,2,4}}}, ::typeof(f!), ::MArray{Tuple{2},Float32,1,2}, ::MArray{Tuple{2},Float32,1,2}, ::ForwardDiff.JacobianConfig{ForwardDiff.Tag{typeof(f!),Float32},Float32,2,Tuple{MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2},MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2}}}) at kawc\ForwardDiff.jl\src\jacobian.jl:170
 [2] jacobian!(::DiffResults.ImmutableDiffResult{1,MArray{Tuple{2},Float32,1,2},Tuple{MArray{Tuple{2,2},Float32,2,4}}}, ::Function, ::MArray{Tuple{2},Float32,1,2}, ::MArray{Tuple{2},Float32,1,2}, ::ForwardDiff.JacobianConfig{ForwardDiff.Tag{typeof(f!),Float32},Float32,2,Tuple{MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2},MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2}}}, ::Val{false}) at kawc\ForwardDiff.jl\src\jacobian.jl:78
 [3] (::NLSolversBase.var"#fj_forwarddiff!#24"{typeof(f!),ForwardDiff.JacobianConfig{ForwardDiff.Tag{typeof(f!),Float32},Float32,2,Tuple{MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2},MArray{Tuple{2},ForwardDiff.Dual{ForwardDiff.Tag{typeof(f!),Float32},Float32,2},1,2}}},MArray{Tuple{2},Float32,1,2}})(::MArray{Tuple{2},Float32,1,2}, ::MArray{Tuple{2,2},Float32,2,4}, ::MArray{Tuple{2},Float32,1,2}) at kawc\NLSolversBase.jl\src\objective_types\oncedifferentiable.jl:160
 [4] value_jacobian!!(::OnceDifferentiable{MArray{Tuple{2},Float32,1,2},MArray{Tuple{2,2},Float32,2,4},MArray{Tuple{2},Float32,1,2}}, ::MArray{Tuple{2},Float32,1,2}, ::MArray{Tuple{2,2},Float32,2,4}, ::MArray{Tuple{2},Float32,1,2}) at kawc\NLSolversBase.jl\src\interface.jl:124
 [5] value_jacobian!! at kawc\NLSolversBase.jl\src\interface.jl:122 [inlined]
 [6] trust_region_(::OnceDifferentiable{MArray{Tuple{2},Float32,1,2},MArray{Tuple{2,2},Float32,2,4},MArray{Tuple{2},Float32,1,2}}, ::MArray{Tuple{2},Float32,1,2}, ::Float32, ::Float32, ::Int64, ::Bool, ::Bool, ::Bool, ::Float32, ::Bool, ::NLsolve.NewtonTrustRegionCache{MArray{Tuple{2},Float32,1,2}}) at kawc\NLsolve.jl\src\solvers\trust_region.jl:119
 [7] trust_region at kawc\NLsolve.jl\src\solvers\trust_region.jl:235 [inlined] (repeats 2 times)
 [8] nlsolve(::OnceDifferentiable{MArray{Tuple{2},Float32,1,2},MArray{Tuple{2,2},Float32,2,4},MArray{Tuple{2},Float32,1,2}}, ::MArray{Tuple{2},Float32,1,2}; method::Symbol, xtol::Float32, ftol::Float32, iterations::Int64, store_trace::Bool, show_trace::Bool, extended_trace::Bool, linesearch::Static, linsolve::NLsolve.var"#27#29", factor::Float32, autoscale::Bool, m::Int64, beta::Int64, aa_start::Int64, droptol::Float32) at kawc\NLsolve.jl\src\nlsolve\nlsolve.jl:26
 [9] nlsolve(::Function, ::MArray{Tuple{2},Float32,1,2}; method::Symbol, autodiff::Symbol, inplace::Bool, kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at kawc\NLsolve.jl\src\nlsolve\nlsolve.jl:52
 [10] top-level scope at REPL[7]:1

After this PR:

julia> using NLsolve
[ Info: Precompiling NLsolve [2774e3e8-f4cf-5e23-947b-6d7e65073b56]

julia> using StaticArrays

julia> function f!(F, x)
          F[1] = (x[1]+3)*(x[2]^3-7)+18
          F[2] = sin(x[2]*exp(x[1])-1)
       end
f! (generic function with 1 method)

julia> FT = Float32;

julia> a = MArray{Tuple{2},FT}(undef);

julia> a .= (0.1, 1.2);

julia> nlsolve(f!, a; autodiff = :forward)
Results of Nonlinear Solver Algorithm
 * Algorithm: Trust-region with dogleg and autoscaling
 * Starting Point: Float32[0.1, 1.2]
 * Zero: Float32[0.1, 1.2]
 * Inf-norm of residuals: 1.656801
 * Iterations: 1000
 * Convergence: false
   * |x - x'| < 0.0e+00: false
   * |f(x)| < 1.0e-08: false
 * Function Calls (f): 2
 * Jacobian Calls (df/dx): 1

[mateuszbaran]

You can also extend this to SizedArray as well (it's also mutable).

[charleskawczynski]

Updated.

[charleskawczynski]

bump

[charleskawczynski]

bump # 2

[charleskawczynski]

bump # 3

[gdalle]

What would you think about #28 instead?

[charleskawczynski]

I’m not sure I have strong opinions. This PR seems a bit simpler. Not sure what to do to get attention to this PR… frankly, I don’t even remember what this was for….

[gdalle]

Okay @devmotion told me why #28 was a bad idea, so I think this PR is better, and I have the rights to merge it, I just have a few comments

[gdalle]

These changes ensure that we don't mess up when value and derivs are of different types. What do you think?

[gdalle]

Just gonna add some tests and we should be good to go

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.84%. Comparing base (0cb922d) to head (38cbfb1).

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #18   +/-   ##
=======================================
  Coverage   86.84%   86.84%           
=======================================
  Files           1        1           
  Lines          76       76           
=======================================
  Hits           66       66           
  Misses         10       10           

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

[gdalle]

That way, if there is even one mutable array in the mix (like MArray), the whole thing becomes a MutableDiffResult

[devmotion]

That's wrong though, isn't it? All value and derivs have to be mutable, otherwise the MutableDiffResult will error when trying to update the immutable component.

[gdalle]

you're right, so it's the other way around

[gdalle]

If any of the `value`/`derivs` is a `Number` or `StaticArrays.SArray`,
then `r` will be immutable. Otherwise, `r` will be mutable.

that's what we want, right?

[devmotion]

Yes, to me it seems that's what we want for StaticArrays. The safest option would be to make the ImmutableDiffResult the default DiffResult type, but given the wide range of mutable arrays (without clear way to catch them all apart from - to some extent - ArrayInterface.ismutable) I think it's much easier implementation-wise to only add special cases for StaticArrays. Even though this is also doomed to fail with wrapper types such as Diagonal etc...

[gdalle]

Theoretically, if everyone used the API correctly, making Immutable the default would not be breaking. Unfortunately, as seen in #26, SciML and others are playing fast and loose with realiasing.

So from an ecosystem point of view, any transition Mutable -> Immutable has the potential to be breaking

[gdalle]

Asking for a review from someone else because these changes could either be considered breaking or a bug fix.

[tpapp]

LGTM

[devmotion]

I think it would be good to run downdtream tests with ReverseDiff, ForwardDiff, etc to avoid accidentally breaking them.

[devmotion]

Oh, the PR was already merged? I think generally we should require approval from people with write permissions (ie JuliaDiff members?).

[gdalle]

Sorry, I'll undo it. How do you suggest we run those downstream tests though, add FD and RD as test deps?

[gdalle]

See #35 for v2