-
-
Notifications
You must be signed in to change notification settings - Fork 71
CaoTauLeaping Implementation #603
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?
CaoTauLeaping Implementation #603
Conversation
extra_keyword_description = """ | ||
- `dtmax`: Maximum allowed step size. | ||
- `dtmin`: Minimum allowed step size. | ||
""", | ||
extra_keyword_default = """ | ||
dtmax = 10.0, | ||
dtmin = 1e-6 |
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.
what are these doing here?
dtmax = 10.0, | ||
dtmin = 1e-6 |
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 don't belong here
if alg.adaptive | ||
integrator.dt = integrator.opts.gamma * integrator.dt / integrator.EEst | ||
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?
# Infer ν_ij using c by applying unit counts for each reaction | ||
num_reactions = length(rate) | ||
ν = zeros(eltype(u), length(u), num_reactions) | ||
unit_counts = zeros(eltype(rate), num_reactions) | ||
for j in 1:num_reactions | ||
unit_counts[j] = 1 | ||
c(ν[:, j], u, p, t, unit_counts, nothing) # ν[:, j] is the change vector for reaction j | ||
unit_counts[j] = 0 # Reset | ||
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.
This is fine for now, but we really should be storing that differently in the jumps. @isaacsas this is worth thinking about.
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.
I think it would be worthwhile to make leaping work with individually defined jumps too. Split-step methods require that anyways.
But that wouldn’t solve this issue for general non-mass action jumps.
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.
and we'd need two implementations of each leaping? I'm thinking the interface should maybe require more information in the jump building, under the assumption users will be using something like Catalyst rather than building most jumps by hand. To do some of this optimally we have to shift been a few different representations, where for a user this would be redundant information to the solver it's different views that have different optimal uses.
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.
For methods that work on the full vector at once, can’t we can manually build the needed function from the list of jumps? So those methods can stick with the current interface, but then we could support methods that really need jump-by-jump rate/affect access too.
But yes, Catalyst should build the needed inputs for different representations.
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.
For methods that work on the full vector at once, can’t we can manually build the needed function from the list of jumps?
You get a lot of compile time trying to inline all of the small functions into the large one. Also there's a lack of fusion. So it's quite slow to do it that way.
I think the point is, some times you need jump-by-jump information, and sometimes you want the aggregate information (all tau leap steppers will use an aggregate function a lot of the time), and those are the same information in two very different computational representations, and building one from the other isn't easy to do from the implementation but it is trivial to do symbolically.
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.
Definitely, so it should be preferred that methods that work on the full vector get a single input function, but having a fallback would be useful. Catalyst/MTK, should just generate both inputs (or whichever is most appropriate for the selected integrator -- we can add a function to query the preferred input type).
for i in eachindex(u) | ||
for j in 1:num_reactions | ||
ν_ij = ν[i, j] | ||
mu[i] += ν_ij * rate[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.
The formula uses the derivative of the rate function, not the rate itself.
ϵ = alg.epsilon | ||
τ_vals = similar(u, Float64) | ||
for i in eachindex(u) | ||
max_term = max(ϵ * u[i], 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.
This is supposed to use the sum of rates term, not u[i], also why the 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.
Oh I see.
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 missing the g_i factor
There are 3 schemes in the paper https://people.cs.vt.edu/~ycao/publication/newstepsize.pdf, can you implement them as different algorithms (reusing most of the code, just different controller dispatches) so we can compare them? |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
Add any other context about the problem here.