DiffEqDiffTools API changes

[dextorious]

This reflects API changes with respect to JuliaDiff/FiniteDiff.jl@4803b91 and passes OrdinaryDiffEq.jl tests.

[devmotion]

I'm not involved in the DiffeqDiffTools development, so maybe it's a stupid question, but why don't you just pass eltype(integrator.u) instead of Val{:Complex} or Val{:Real}? That would simplify the changes here a lot; you could just add another method to DiffeqDiffTools which computes Val{:Complex} and Val{:Real} and calls the currently implemented functions.

[devmotion]

We could go even further and implement

DiffEqDiffTools.derivative!(dT, tf, du2, t, integrator)

(or something similar), which depending on alg_autodiff(integrator) either calls ForwardDiff.derivative!(dT, tf, vec(du2), t) or DiffEqDiffTools.finite_difference!(dT, tf, t, integrator.alg.diff_type, eltype(integrator.u), Val{:DiffEqDerivativeWrapper}, vec(du2)) (or the already implemented method with Val{:Complex} and Val{:Real}). An analogous change could be applied to the calculation of Jacobians.

These changes would simplify the code in OrdinaryDiffEq but maybe derivative! (and similar methods) should be part of OrdinaryDiffEq rather than of DiffEqDiffTools?

[dextorious]

That's a question of how much flexibility @ChrisRackauckas prefers to retain on the OrdinaryDiffEq side (and, eventually, other DiffEq packages). I kept the checks on the OrdinaryDiffEq side of things because I assumed Chris might want to later expose the choice between real and complex in the user-facing API for explicit overriding. If that's not viewed as necessary I have no objection to moving as much of the boilerplate as possible over to DiffEqDiffTools and simplifying the calls used in OrdinaryDiffEq along the lines you suggested.

[devmotion]

Yes, that's what I was wondering about, hence I assumed it might be desired to keep the form with Val{:Complex} and Val{:Real}. As you mention, depending on the intention behind and plans with DiffEqDiffTools we maybe do not want to add the suggested methods to DiffEqDiffTools. However, we could still add them to OrdinaryDiffEq; but that's a question that is up to Chris and you, I guess.

By the way, a method like DiffEqDiffTools.derivative!(dT, tf, du2, t, integrator) would still allow to add user specifications of real and complex (similar to the currently used integrator.alg.diff_type) without much effort, by just calling the Val form explicitly after some computations/considerations. Of course, the form with eltype(integrator.u) would not be required then.

[dextorious]

Yeah, my response was mostly about the eltype(integrator.u) part and the way that's currently handled. We can easily dress this up in wrappers like you suggested.

Broadly speaking, my goal with DiffEqDiffTools was just to write the low level stuff in a way that delivers close to optimal performance without sacrificing flexibility. That's pretty much done now, at the cost of a rather ugly and verbose API. This commit just uses it directly, in order to get the features out there ASAP, since other people are waiting for them. I'm happy to write up any convenience wrappers people would like to see, but any DiffEq API level decisions are up to Chris, so my PRs just expose the new features without making any assumptions about the higher level stuff. For what it's worth, I like the DiffEqDiffTools.derivative!(dT, tf, du2, t, integrator) idea, but it does introduce an implicit API dependency between the packages.

[ChrisRackauckas]

I'm not involved in the DiffeqDiffTools development, so maybe it's a stupid question, but why don't you just pass eltype(integrator.u) instead of Val{:Complex} or Val{:Real}? That would simplify the changes here a lot; you could just add another method to DiffeqDiffTools which computes Val{:Complex} and Val{:Real} and calls the currently implemented functions.

This was done for the Optim.jl guys who always want a real output, say from the cost equation, which may have complex inputs. We won't have this problem, but we hope to replace the Calculus.jl they use to help them get some more speed and abstract type support.

By the way, a method like DiffEqDiffTools.derivative!(dT, tf, du2, t, integrator) would still allow to add user specifications of real and complex (similar to the currently used integrator.alg.diff_type) without much effort, by just calling the Val form explicitly after some computations/considerations. Of course, the form with eltype(integrator.u) would not be required then.

That's probably a nice way to handle this. This could even just be defined on the abstract integrator from DiffEqBase and it'll work then in other packages like StochasticDiffEq.jl as well.

I'm happy to write up any convenience wrappers people would like to see, but any DiffEq API level decisions are up to Chris, so my PRs just expose the new features without making any assumptions about the higher level stuff. For what it's worth, I like the DiffEqDiffTools.derivative!(dT, tf, du2, t, integrator) idea, but it does introduce an implicit API dependency between the packages.

API refactoring can always be done later (and it seems like @devmotion came up with a good solution for it), so I am not making it a necessity. What I want to see more is speed + complex number and abstract array support, which this gives, so I'm happy to take it and maybe implement the cleaner form in a different PR (unless you're willing to make the change). But I think that while I have your attention I'd rather try to push forward on JuliaDiff/FiniteDiff.jl#16 and JuliaDiff/FiniteDiff.jl#13 . Of course, as always with open source, choose what you want to spend your time with, but I am pushing towards solving the methods issues first because API refactoring is something that can always be done on a flight or as an intro issue for a GSoC student (or one of us when we're bored can just bang it out in 20 minutes).

[dextorious]

Now that I have your okay on the API thing, that's a very easy change. The fj! issue is also quick, but Hessians will take quite a bit more time and I'd rather leave that to a separate PR. I do intend to deal with all three, the first two hopefully today. The Hessians are a slightly lower priority, both because they will take more time and also because the NLSolve changes we've been looking forward to don't hinge on that feature (correct me if I'm wrong).

[ChrisRackauckas]

The Hessians are a slightly lower priority, both because they will take more time and also because the NLSolve changes we've been looking forward to don't hinge on that feature (correct me if I'm wrong).

Yes, NLsolve doesn't need them but Optim.jl needs them to fully dump Calculus.jl

[dextorious]

Okay, then I'll try to get the API and fj! stuff done today, so we can merge that and make Hessians my next priority. If Optim.jl is using Calculus.jl for that currently, they probably only need Hessians of real-valued functions to start with, but I'll do both when I get the time.

[dextorious]

Wrote up quick convenience wrappers for derivatives and Jacobians as per @devmotion's suggestion. It temporarily resides in OrdinaryDiffEq and should be moved over to DiffEqDiffTools along with the derivative wrapper types. As we've discussed previously, I'm holding off on such refactoring until the major features are done, but this should at least make writing and maintaining integrators a bit less painful.

[devmotion]

I like these convenience wrappers 👍

It just feels a bit inconsistent that these wrappers allow arguments of type AbstractArray although apparently only AbstractVector should be specified (e.g. in case of du2 which therefore is always provided as vec(du2)). In my opinion, either these arguments should be required to be of type AbstractVector or (preferably) the convenience wrappers should hide this requirement and internally apply vec (then derivative! could be called with du2). But such a change can also be part of a later refactoring, I guess.

[devmotion]

Or is it even possible to remove those vec altogether? ForwardDiff seems to work on AbstractArray and also DiffEqDiffTools seems to allow arbitrary AbstractArray, as far as I see.

[dextorious]

You're right, I need to tighten up the type constraints both here and on the DiffEqDiffTools side as well. At least on the latter, they should be AbstractVector. Regarding the vec calls, I believe @ChrisRackauckas had a fairly subtle reason for them originally for dealing with exotic types, so I'll let him answer that. I'm fine with at least moving them over into the convenience wrappers.

[ChrisRackauckas]

I haven't been able to review yet but after the ForwardDiff change we shouldn't need to vec and reshape anymore.

[ChrisRackauckas]

I reviewed it. It's already a lot of changes in the right direction, so I think it's good to accept it and keep iterating on it instead of trying to get it all in one perfect PR.

[ChrisRackauckas]

This is a really clean way to handle all of this. Nice.

[coveralls]

Coverage increased (+0.07%) to 76.817% when pulling f6e7a6b on dextorious:pull-request/31e05ab5 into 4cd4730 on JuliaDiffEq:master.

[codecov]

Codecov Report

Merging #224 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
+ Coverage   76.74%   76.81%   +0.07%     
==========================================
  Files          61       61              
  Lines       21476    21434      -42     
==========================================
- Hits        16482    16465      -17     
+ Misses       4994     4969      -25

Impacted Files	Coverage Δ
src/derivative_wrappers.jl	96.29% <100%> (+8.06%)	⬆️
src/integrators/rosenbrock_integrators.jl	93.28% <100%> (+0.97%)	⬆️
src/integrators/sdirk_integrators.jl	64.88% <100%> (+0.09%)	⬆️

---

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 4cd4730...f6e7a6b. Read the comment docs.