Add DiffResult for MArray #18

charleskawczynski · 2020-10-31T15:27:38Z

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 · 2020-11-10T22:21:29Z

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

charleskawczynski · 2020-11-10T22:34:29Z

Updated.

charleskawczynski · 2020-11-16T19:27:02Z

bump

charleskawczynski · 2020-12-07T03:50:56Z

bump # 2

charleskawczynski · 2021-02-09T16:13:18Z

bump # 3

gdalle · 2024-03-15T18:27:26Z

What would you think about #28 instead?

charleskawczynski · 2024-03-19T00:07:09Z

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 · 2024-03-19T05:58:39Z

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?

src/DiffResults.jl

gdalle · 2024-03-19T06:05:11Z

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

…i/18

codecov · 2024-03-19T06:09:12Z

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 · 2024-03-19T06:41:52Z

src/DiffResults.jl

@@ -36,7 +36,7 @@ Return `r::DiffResult`, with output value storage provided by `value` and output
 storage provided by `derivs`.

 In reality, `DiffResult` is an abstract supertype of two concrete types, `MutableDiffResult`
-and `ImmutableDiffResult`. If all `value`/`derivs` are all `Number`s or `StaticArray`s,
+and `ImmutableDiffResult`. If all `value`/`derivs` are all `Number`s or `StaticArrays.SArray`s,


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

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.

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

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?

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...

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 · 2024-03-19T06:46:49Z

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 · 2024-03-19T09:33:32Z

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

gdalle · 2024-03-19T09:38:35Z

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

This reverts commit f80602e.

This reverts commit 91614bb.

gdalle · 2024-03-19T14:08:52Z

See #35 for v2

Add DiffResult for MArray

9524f62

Add DiffResult for SizedArray

f7c8877

pkofod mentioned this pull request Dec 16, 2020

Cannot nlsolve with MArray JuliaNLSolvers/NLsolve.jl#254

Open

gdalle linked an issue Mar 15, 2024 that may be closed by this pull request

MVector type yields ImmutableDiffResult #25

Open

gdalle requested changes Mar 19, 2024

View reviewed changes

src/DiffResults.jl Outdated Show resolved Hide resolved

src/DiffResults.jl Outdated Show resolved Hide resolved

gdalle added 2 commits March 19, 2024 07:04

Update src/DiffResults.jl

2b16fe9

Update src/DiffResults.jl

c25b292

gdalle added 2 commits March 19, 2024 07:07

Merge remote-tracking branch 'origin/master' into pr/charleskawczynsk…

973f7c8

…i/18

Merge master

5160396

gdalle added 4 commits March 19, 2024 07:39

Add tests

061a09c

Ignore Manifest

4670f89

Undo gitignore

356773b

Noise in changes

141adbc

gdalle reviewed Mar 19, 2024

View reviewed changes

gdalle added 2 commits March 19, 2024 07:42

Remove noise

5ba9c6b

Remove more noise

d3fecde

gdalle requested a review from devmotion March 19, 2024 06:46

tpapp approved these changes Mar 19, 2024

View reviewed changes

Bump version to 1.2

38cbfb1

gdalle merged commit f80602e into JuliaDiff:master Mar 19, 2024
6 checks passed

devmotion reviewed Mar 19, 2024

View reviewed changes

gdalle added a commit that referenced this pull request Mar 19, 2024

Revert "Add DiffResult for MArray (#18)"

15f042d

This reverts commit f80602e.

gdalle mentioned this pull request Mar 19, 2024

Revert "Add DiffResult for MArray" #32

Merged

gdalle added a commit that referenced this pull request Mar 19, 2024

Revert "Add DiffResult for MArray (#18)" (#32)

91614bb

This reverts commit f80602e.

gdalle mentioned this pull request Mar 19, 2024

Downstream tests #33

Closed

gdalle added a commit that referenced this pull request Mar 19, 2024

Revert "Revert "Add DiffResult for MArray (#18)" (#32)"

80cb13c

This reverts commit 91614bb.

devmotion mentioned this pull request Mar 19, 2024

Add DiffResult for MArray, take 2 #35

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DiffResult for MArray #18

Add DiffResult for MArray #18

charleskawczynski commented Oct 31, 2020 •

edited

Loading

mateuszbaran commented Nov 10, 2020

charleskawczynski commented Nov 10, 2020

charleskawczynski commented Nov 16, 2020

charleskawczynski commented Dec 7, 2020

charleskawczynski commented Feb 9, 2021

gdalle commented Mar 15, 2024

charleskawczynski commented Mar 19, 2024

gdalle commented Mar 19, 2024

gdalle left a comment

gdalle commented Mar 19, 2024

codecov bot commented Mar 19, 2024 •

edited

Loading

gdalle Mar 19, 2024

devmotion Mar 19, 2024

gdalle Mar 19, 2024

gdalle Mar 19, 2024

devmotion Mar 19, 2024

gdalle Mar 19, 2024

gdalle commented Mar 19, 2024

tpapp left a comment

devmotion left a comment

devmotion commented Mar 19, 2024

gdalle commented Mar 19, 2024 •

edited

Loading

gdalle commented Mar 19, 2024

Add DiffResult for MArray #18

Add DiffResult for MArray #18

Conversation

charleskawczynski commented Oct 31, 2020 • edited Loading

mateuszbaran commented Nov 10, 2020

charleskawczynski commented Nov 10, 2020

charleskawczynski commented Nov 16, 2020

charleskawczynski commented Dec 7, 2020

charleskawczynski commented Feb 9, 2021

gdalle commented Mar 15, 2024

charleskawczynski commented Mar 19, 2024

gdalle commented Mar 19, 2024

gdalle left a comment

Choose a reason for hiding this comment

gdalle commented Mar 19, 2024

codecov bot commented Mar 19, 2024 • edited Loading

Codecov Report

gdalle Mar 19, 2024

Choose a reason for hiding this comment

devmotion Mar 19, 2024

Choose a reason for hiding this comment

gdalle Mar 19, 2024

Choose a reason for hiding this comment

gdalle Mar 19, 2024

Choose a reason for hiding this comment

devmotion Mar 19, 2024

Choose a reason for hiding this comment

gdalle Mar 19, 2024

Choose a reason for hiding this comment

gdalle commented Mar 19, 2024

tpapp left a comment

Choose a reason for hiding this comment

devmotion left a comment

Choose a reason for hiding this comment

devmotion commented Mar 19, 2024

gdalle commented Mar 19, 2024 • edited Loading

gdalle commented Mar 19, 2024

charleskawczynski commented Oct 31, 2020 •

edited

Loading

codecov bot commented Mar 19, 2024 •

edited

Loading

gdalle commented Mar 19, 2024 •

edited

Loading