replace ForwardDiff.jacobian with SparseDiffTools.forwarddiff_color_jacobian #937
Conversation
src/derivative_wrappers.jl
Outdated
| jacobian_autodiff(f, x) = (ForwardDiff.derivative(f,x),1) | ||
| jacobian_autodiff(f, x::AbstractArray) = (ForwardDiff.jacobian(f, x),1) | ||
| jacobian_autodiff(f, x, integrator) = (ForwardDiff.derivative(f,x),1) | ||
| function jacobian_autodiff(f, x::AbstractArray, integrator) |
There was a problem hiding this comment.
Why is integrator passed as argument? It seems only integrator.f is accessed in the function body - shouldn't these calls be replaced by f, in which case integrator would be needless?
There was a problem hiding this comment.
I think it is just a legacy.
There was a problem hiding this comment.
I don't see how passing integrator.f instead of integrator addresses the point that I wanted to make? IMO there should be only two arguments to jacobian_autodiff.
There was a problem hiding this comment.
Because it is colored jacobian.
There was a problem hiding this comment.
Sorry, I don't quite understand. My question is: why can we not replace all occurrences of odefun by f?
There was a problem hiding this comment.
Oh, I thought f is the unwrapped function from ODEFunction object.
There was a problem hiding this comment.
Well, it turns out the f is indeed the unwrapped function. I replaced odefun with f and got type UDerivativeWrapper has no field jac_prototype.
There was a problem hiding this comment.
Ah I see, thanks for trying! I'm wondering if it would make sense to add more traits and getproperty overloads to UDerivativeWrapper and UJacobianWrapper (as done in https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/c65c0e7442b63332aac60002baa9ebb021890e76/src/diffeqfunction.jl#L883-L888) instead of passing integrator.f
There was a problem hiding this comment.
I guess that's not very friendly for extension unless there is a generic way to do it.
Codecov Report
@@ Coverage Diff @@
## master #937 +/- ##
==========================================
+ Coverage 77.18% 77.19% +0.01%
==========================================
Files 97 97
Lines 30636 30609 -27
==========================================
- Hits 23645 23629 -16
+ Misses 6991 6980 -11
Continue to review full report at Codecov.
|
huanglangwen
force-pushed
the
colorjac
branch
from
fd8509e to
9846b8d
Compare
huanglangwen
force-pushed
the
colorjac
branch
from
42afd5f to
c430f47
Compare
|
Currently , this pr fails five tests where cache tests and resize tests are related with the missing if !is_APPVEYOR && (GROUP == "All" || GROUP == "Integrators_I")
@time @safetestset "Alg Events Tests" begin include("integrators/alg_events_tests.jl") end
@time @safetestset "Cache Tests" begin include("integrators/ode_cache_tests.jl") end
@time @safetestset "Add Steps Tests" begin include("integrators/ode_add_steps_tests.jl") end
end
if !is_APPVEYOR && (GROUP == "All" || GROUP == "Integrators_II")
@time @safetestset "Reverse Directioned Event Tests" begin include("integrators/rev_events_tests.jl") end
@time @safetestset "Resize Tests" begin include("integrators/resize_tests.jl") end
end |
|
It seems the problems concentrate on callbacks and jaccache resize, I have no idea how to solve this. @ChrisRackauckas |
|
Alright I'm taking a look. |
4 participants
No description provided.