add sparsediff #202
add sparsediff #202
Conversation
|
Will be interesting to see if this leads to similar compilation time regressions as the ones we observed in OrdinaryDiffEq and DelayDiffEq after the addition of SparseDiffTools. |
|
Actually, the sparsediff functionality only depends on SparsediffTool's autodiff jacobian with colors which is rather independent from other parts. |
|
In this discourse post I discuss how DelayDiffEq suddenly became completely unusable on Win32 and how the compilation on Win64 and Linux 64 bit increased by roughly 3 minutes, and how this change is clearly related to the PR that added SparseDiffTools to OrdinaryDiffEq. There is some fundamental problem with the integration of SparseDiffTools in OrdinaryDiffEq that I do not understand yet. Maybe it's not even caused by SparseDiffTools but by some dependencies it's pulling in. |
src/derivative_wrappers.jl
Outdated
| colorvec=integrator.f.colorvec | ||
| sparsity=integrator.f.jac_prototype | ||
| jac=integrator.f.jac | ||
| J=jac isa SparseMatrixCSC ? similar(jac) : zeros(size(jac)...) |
There was a problem hiding this comment.
No need for splatting here, it seems.
There was a problem hiding this comment.
Sorry, what's splatting?
There was a problem hiding this comment.
Splatting is the ..., and it leads to bad interactions with the compiler. It should be avoided whenever possible. Here it's not necessary.
There was a problem hiding this comment.
Oh, find an error. jac should be f.jac_prototype instead of f.jac which is a function. I was thinking about cache.J since cache.J sometimes is different from jac_prototype , but looks like it is not available here. I need to fix OrdDiffEq as well.
There was a problem hiding this comment.
src/derivative_wrappers.jl
Outdated
| else | ||
| J = ForwardDiff.derivative(f,x) | ||
| end | ||
| J = jacobian_autodiff(f,x,integrator,Val{DiffEqBase.has_colorvec(integrator.f)}) |
There was a problem hiding this comment.
I'm wondering if introducing those functions and dispatching is useful at all. Although I think that should not put too much stress on the compiler I'm looking for every tiny bit that could possibly explain a slowdown 😛 I just read through the documentation (https://docs.julialang.org/en/v1/manual/types/index.html#%22Value-types%22-1 and https://docs.julialang.org/en/v1/manual/performance-tips/#The-dangers-of-abusing-multiple-dispatch-(aka,-more-on-types-with-values-as-parameters)-1) and I'm worried that it's not useful at all to dispatch here. Additionally, according to the documentation one should rather use Val(...) instances for consistency 😮 I had never heard this before.
There was a problem hiding this comment.
Well that's suggested by @ChrisRackauckas to reduce runtime brunch. And yeah, I should use Val(), sorry I didn't read docs carefully.
There was a problem hiding this comment.
Yes, this is an inproper use of Val though. The chunksize needs to be saved as a type, otherwise this will dynamic dispatch every time and be slow unless it infers. Why does this part of the code make a Val?
I don't think this is blocking though and can get cleaned up later.
src/derivative_wrappers.jl
Outdated
| function DiffEqBase.build_jac_config(alg::StochasticDiffEqAlgorithm,f,uf,du1,uprev,u,tmp,du2) | ||
| if !has_jac(f) | ||
| if alg_autodiff(alg) | ||
| jac_config = ForwardDiff.JacobianConfig(uf,du1,uprev,ForwardDiff.Chunk{determine_chunksize(u,alg)}()) | ||
| jac_config = jac_cache_autodiff(alg,f,uf,du1,uprev,u,Val{DiffEqBase.has_colorvec(f)}) |
src/StochasticDiffEq.jl
Outdated
| @@ -47,6 +47,8 @@ module StochasticDiffEq | |||
|
|
|||
| import DiffEqBase: iip_get_uf, oop_get_uf, build_jac_config | |||
|
|
|||
| import SparseDiffTools: forwarddiff_color_jacobian!, ForwardColorJacCache | |||
There was a problem hiding this comment.
I guess using is more appropriate? (see https://docs.julialang.org/en/v1/manual/modules/#Summary-of-module-usage-1)
|
Already the first Travis tests indicate that this PR leads to severe compilation time regressions: a comparison of the times of the first tests in the different test groups for the latest commit on the master branch with the times for this commit reveals a consistent increase of around 2-3 minutes. In particular, if this is supposed to be a small change that is way too much IMO. I assume, on Win32 it will be much worse. |
|
Apparently there are some issues with SparseDiffTools even after getting rid of all |
|
The real performance issue is including sparsedifftools led to inclusion of many other packages like Zygote. It is possible to get rid of the sparsedifftools dependency once merging colored jacobian into ForwardDiff. |
Yes, and that's the plan, but it'll take some time. So for now I think we are just going to |
No description provided.