Fast remake

[tkf]

Let's track performance issue with remake here. But, at the moment, the problem is still in the inner constructor since the @inferred tests, e.g.,

https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/b3260af1545c13351e334a29f1868a28f9aa6ff1/test/problem_creation_tests.jl#L20-L21

are still broken.

(I just noticed that @ChrisRackauckas mentioned performance issue with remake in #114 so opening an issue here.)

[tkf]

One fix we can do without waiting for the compiler to be improved is to switch to @jw3126's https://github.com/jw3126/Setfield.jl which internally uses positional-only constructor. So it was type-stable even with Julia 0.6. Setfield.jl is based on the abstraction of getter and setter called lens (see Setfield.Lens) and supports not only changing struct fields but also elements of (possibly immutable) arrays. It means that something like this works

new_prob = @set prob.u0[1] = 1.0

provided that we define positional-only constructors for all structs. Also, I think we can get rid of all solution_new_retcode etc. and instead use

integrator.sol = @set sol.code = code

Lens is well-defined and composable. It's developed in the Haskell world so I guess there is no wonder why :) https://github.com/ekmett/lens

(I found that this approach was great so soon after Setfield.jl is released I ditched my Reconstructables.jl, which was a spin-off package of remake, and then used @lens as the primary API for specifying varying parameters in Bifurcations.jl.)

But there are some caveats (although I think they are solvable):

1. Handling mixture of mutable and immutable data, such as integrator.sol, is tricky. We had @set! in the current Setfield.jl v0.1.0 release but it will be removed in the next release. Previously you could write

@set! integrator.sol.code = code

for creating new sol but mutating integrator in-place. I think we probably can bring @set! back but it requires a bit more experiments and design iterations.

2. Setting multiple fields is tricky. What is equivalent to remake(prob; u0=u0, tspan=tspan) is

prob1 = @set prob.u0 = u0
prob2 = @set prob1.tspan = tspan

which is a bit cumbersome. Also, since @set prob.u0 = u0 may change the type parameter of prob, you need to use a different name for prob1 than prob2 if you want type-stability. Furthermore, if the inner constructor has to do some checks, it will be run multiple types. We are still discussing how to support it jw3126/Setfield.jl#23 (comment)

[ChrisRackauckas]

Yes thanks for opening this. This is definitely one of the big issues right now.

[tkf]

You listed it in SciML/DifferentialEquations.jl#318 which means API change. So you want to switch to Setfield.jl?

Also, I think we can (temporary) have both @set and remake which means that we can add it during 5.x. I just don't want make it a blocker.

[ChrisRackauckas]

I listed it in SciML/DifferentialEquations.jl#318 because I was hoping we could have this done in time. I do want to switch to SetField.jl and get it inferring well.

But I don't get what's so bad about the keyword argument methods. While Julia doesn't dispatch on keyword arguments, it now specializes on them: what are we doing that is not allowing this?

[tkf]

One thing we get from SetField "for free" is the nested-field support. If we make DiffEqBase SetField-compatible we can do:

old :: ODEProblem
new = @set old.f.jac = new_jacobian
new :: ODEProblem

Technically we can do it with remake by making a macro which does AST-to-AST conversion. This AST-to-AST conversion was actually already implemented in https://github.com/tkf/Reconstructables.jl/tree/master for Julia 0.6 but maintaining complex macro is not great. Also, I feel like lens-based approach taken by Setfield is much better since it can do:

new = @set old.u0[1] = 1.0

when u0 is a SVector.

While Julia doesn't dispatch on keyword arguments, it now specializes on them: what are we doing that is not allowing this?

Yeah, I was surprised that @inferred ODEProblem(f,u0,tspan) was still broken with Julia 1.0. Probably there is something to be fixed in the inner constructor. If that's the case, using Setfield or not doesn't matter.

BTW, just for curiosity: what's the usecase where remake becomes the bottleneck? Random sampling?

[tkf]

Ah, so the @inferred tests are failing because we are feeding "raw" Julia functions. Then ODEProblem calls convert(::Type{ODEFunction{iip}}, f) which has a lot of type-unstable (for good) branches:

https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/b3260af1545c13351e334a29f1868a28f9aa6ff1/src/diffeqfunction.jl#L530-L536

If I create ODEFunction manually first, @inferred works:

julia> f = (u, p, t) -> -u; u0 = [1.0, -0.5]; tspan = (0.0, 5.0);

julia> of = ODEFunction(f);

julia> @inferred ODEProblem(of, u0, tspan)
ODEProblem with uType Array{Float64,1} and tType Float64. In-place: false
timespan: (0.0, 5.0)
u0: [1.0, -0.5]

[ChrisRackauckas]

Oh okay, so remake just needs to use the ODEFunction as the default?

[tkf]

I think there are three cases:

1. If you are not changing f, remake should be type stable because prob.f is already converted to an AbstractODEFunction.

2. If you are setting f which is already an AbstractODEFunction, remake should be type stable.

3. If you are setting f which is not an AbstractODEFunction, then remake is not type stable because f has to be converted.

(But I still need to catch up to the recent changes in DiffEqBase so I may be missing something.)

[ChrisRackauckas]

Okay, is that already true? I'll check this. If it's true, then we can consider this already done.

[tkf]

Sorry, it still does not work:

julia> @inferred remake(prob, u0=[10.0, 10.0])
ERROR: return type ODEProblem{Array{Float64,1},Tuple{Float64,Float64},false,Nothing,ODEFunction{false,getfield(Main, Symbol("##13#14")),LinearAlgebra.UniformScaling{Bool},Nothing,Nothing,Nothing,Nothing,Nothing,Nothing,Nothing,Nothing},Nothing,DiffEqBase.StandardODEProblem} does not match inferred return type Any

and I forgot that we have type-unstable branch in remake:

https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/b547e71d838bb1ea4c3bb525bd03192db94e0669/src/utils.jl#L173-L183

We tried to remove it in #89 using @genereted but the effect was not clear (as we had the issue in constructor back then).

We can try the @genereted way but hasmethod is not pure so it's not legit to use it in generated function IIUC.

[tkf]

https://discourse.julialang.org/t/can-i-use-hasmethod-inside-generated-function/13645?u=tkf