-
-
Notifications
You must be signed in to change notification settings - Fork 38
SimpleAdaptiveTauLeaping solver #513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
SimpleAdaptiveTauLeaping solver #513
Conversation
Rebase onto master |
f266a9d
to
5963f3e
Compare
@sivasathyaseeelan can you make the correctness test more strict. Compare against I’m reading through the references to refresh my memory on how the adaptive stepping works, and will try to finish that tomorrow and give you some more feedback afterwards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sivasathyaseeelan, to speed up review I suggest splitting this into separate PRs for explicit vs. implicit. Let's get explicit merged and then we can work on implicit.
@ChrisRackauckas I think there are some design decisions to be made here for getting at the individual rate functions and stochiometry vectors. So it is just a question if you want to work on that as part of these or as a followup -- I'd defer to what you want, but I don't think these should be advertised until we get that settled and have more extensive testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comments:
- Please add comments citing the formulas you are using from the original paper for the leap selection.
- Please revise with an eye towards making this non-allocating during the solution timestepping, see the examples I've highlighted but keep in mind they are not all the cases of extraneous allocations.
SimpleAdaptiveTauLeaping(; epsilon=0.05) = SimpleAdaptiveTauLeaping(epsilon) | ||
|
||
function compute_gi(u, nu, hor, i) | ||
max_order = 1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just make this an integer? It should just be an integer right given that hor
is integer?
Generally try not to assume Float64
types in your code. They should be inferred from the types of t
or the type of the elements of u
as appropriate.
# Compute initial stoichiometry and HOR | ||
nu = zeros(Int, length(u0), numjumps) | ||
counts_temp = zeros(Int, numjumps) | ||
for j in 1:numjumps | ||
fill!(counts_temp, 0) | ||
counts_temp[j] = 1 | ||
c(du, u0, p, t[1], counts_temp, nothing) | ||
nu[:, j] = du | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the other PR, I suggest switching to the input being MassActionJump
s. Then you can get rid of most of this stoichiometry-related code. It will also have a big performance benefit since you won't need to recalculate nu
every timestep (which is likely to be very expensive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once you've made the switch to MassActionJump
s you can get the stoichiometry for the i
th reaction from net_stoch[i]
. It only needs to be extracted once as it doesn't change during a simulation.
end | ||
hor = zeros(Int, size(nu, 2)) | ||
for j in 1:size(nu, 2) | ||
hor[j] = sum(abs.(nu[:, j])) > maximum(abs.(nu[:, j])) ? 2 : 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this need to be calculated based on the substrate stoichiometry, not the net stoichiometry? (The order of a reaction is determined by the substrates coefficients, i.e. for MassActionJump
s you can get these from the reactant_stoch
field.
hor[j] = sum(abs.(nu[:, j])) > maximum(abs.(nu[:, j])) ? 2 : 1 | ||
end | ||
|
||
saveat_times = isnothing(saveat) ? Float64[] : saveat isa Number ? collect(range(tspan[1], tspan[2], step=saveat)) : collect(saveat) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
saveat_times = isnothing(saveat) ? Float64[] : saveat isa Number ? collect(range(tspan[1], tspan[2], step=saveat)) : collect(saveat) | |
saveat_times = isnothing(saveat) ? Vector{typeof(t))() : saveat isa Number ? collect(range(tspan[1], tspan[2], step=saveat)) : collect(saveat) |
This is too long to read / parse, expand it out with an outer if/else block. Try not to nest ternary operators as it gets hard to read the code.
u = [copy(u0)] | ||
t = [tspan[1]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make these the current u
and current t
, and have separate vectors usave
and tsave
or such for the history. That is more consistent with SciML naming.
# Interpolate to saveat times if specified | ||
if !isempty(saveat_times) | ||
t_out = saveat_times | ||
u_out = [u[end]] | ||
for t_save in saveat_times | ||
idx = findlast(ti -> ti <= t_save, t) | ||
push!(u_out, u[idx]) | ||
end | ||
t = t_out | ||
u = u_out[2:end] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? Aren't you directly stepping to saveat
times?
max_order = max(max_order, Float64(hor[j])) | ||
end | ||
end | ||
return max_order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't necessarily the correct value for hor[i] > 1
. If hor[i] == 2
you need to precalculate if any reaction for which species i
is a substrate has the substrate stoichoimetry of 2 or more (using reactant_stoch
for example). Then you need to use the formulate for the updated g_i
in case (II). Similar for if hor[i] ==3
.
We don't need to support if hor[i] > 3
, but the solver should probably error if it detects this is the case.
mu = zeros(length(u)) | ||
sigma2 = zeros(length(u)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid allocations. Can't these just be scalar variables within the loop over i
?
mu[i] += nu[i, j] * rate_cache[j] | ||
sigma2[i] += nu[i, j]^2 * rate_cache[j] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These can just be written as dot products using views of nu
.
@test isapprox(mean_direct_S[i], mean_simple_S[i], rtol=0.10) | ||
@test isapprox(mean_direct_S[i], mean_adaptive_S[i], rtol=0.10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is too loose a bound. At least decrease it to 5%.
max_order = 1.0 | ||
for j in 1:size(nu, 2) | ||
if abs(nu[i, j]) > 0 | ||
max_order = max(max_order, Float64(hor[j])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
max_order = max(max_order, Float64(hor[j])) | |
max_order = max(max_order, float(hor[j])) |
struct SimpleAdaptiveTauLeaping <: DiffEqBase.DEAlgorithm | ||
epsilon::Float64 # Error control parameter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
struct SimpleAdaptiveTauLeaping <: DiffEqBase.DEAlgorithm | |
epsilon::Float64 # Error control parameter | |
struct SimpleAdaptiveTauLeaping{T <: AbstractFloat} <: DiffEqBase.DEAlgorithm | |
epsilon::T # Error control parameter |
SimpleAdaptiveTauLeaping Plot:
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.