WIP: Add inplace version of cvode #76
Conversation
|
See #75 |
|
Personally I don't see any reason except being backward compatible. So since #75 discusses how to improve the output, I would suggest to also transpose the output matrix (if the matrix ouput is kept). |
|
See #87. This has in-place versions. The reason why the samples were stored along rows instead of columns is because Julia is column-major. If you write to a row, you're not actually iterating consecutive terms and it can cost you performance (you're jumping columns so the pointer has to jump, and each time you go a row down, the further it has to jump). This is one of many reasons why |
src/simple.jl
Outdated
| integrator=:BDF, reltol::Float64=1e-3, abstol::Float64=1e-6) = | ||
| cvode!(f, y0, t, zeros(length(t), length(y0)), userdata; | ||
| integrator=integrator, reltol=reltol, abstol=abstol) | ||
|
|
||
| function cvode!(f::Function, y0::Vector{Float64}, t::Vector{Float64}, y::Matrix{Float64}, userdata::Any=nothing; | ||
| function cvode!(f::Function, y0::Vector{Float64}, t::AbstractVector, y::Matrix{Float64}, userdata::Any=nothing; |
There was a problem hiding this comment.
I'm pretty sure Sundials needs Vector{Float64}s since it's doing C interop. Have you tested other AbstractVectors?
There was a problem hiding this comment.
Looks like t is not passed into Sundials, it's just for iterating timepoints within cvode!(). OTOH it would make sense to declare it as AbstractVector{S}, S<:AbstractFloat.
| cvode!(f, y0, t, zeros(length(t), length(y0)), userdata; | ||
| integrator=integrator, reltol=reltol, abstol=abstol) | ||
|
|
||
| function cvode!(f::Function, y0::Vector{Float64}, t::AbstractVector, y::Matrix{Float64}, userdata::Any=nothing; |
There was a problem hiding this comment.
By convention, !-functions modify their 1st argument, so I would recommend putting y forward.
Also, the size of y has to be checked and DimensionMismatch thrown if it doesn't meet expectation.
With this check done, you can annotate the for k loop with @inbounds.
There was a problem hiding this comment.
I think @alyst 's note about the function signature order is the only remaining problem before merging. Is the author still around for this? Or should I just merge and make this quick change?
| cvode!(f, y0, t, zeros(length(t), length(y0)), userdata; | ||
| integrator=integrator, reltol=reltol, abstol=abstol) | ||
|
|
||
| function cvode!(f::Function, y0::Vector{Float64}, t::AbstractVector, y::Matrix{Float64}, userdata::Any=nothing; |
There was a problem hiding this comment.
I think @alyst 's note about the function signature order is the only remaining problem before merging. Is the author still around for this? Or should I just merge and make this quick change?
|
The rebased this, did the changes, and merged to master. Thanks for the contribution. |
Hi all,
Due to the recent activity on this package I decided to scratch a few itches...
In my code I somehow have already allocated the output where I want to store the solution of some differential equation. This PR adds a method
cvode!that accept an input matrix where the solution can be stored. There is also the usual methodcvodethat allocates the output in case it is needed.Also, I could not resist relaxing the typing for the time vector. So many times I have
tas aFloatRangeor alinspace...Could anyone explain the historical reason why we store samples of the solution along rows of the output and not along columns in an array of transposed sizes?