-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Spatial Reaction Network Implementation #644
Conversation
Updated using improved jacobian, which should make scaling decent (although something still seems to happen around 10,000 compartments that I do not understand). Syntax is basically the same, with the exception that the only type of spatial reaction is the normal diffusion reaction (where a species moves from one compartment to an adjacent one): |
This PR is probably as good as it can get now, the only possible addition is documentation (which I will add once we are happy with it. Around n=10,000 nodes, performance for implicit ODE solvers starts to drop (for the brusselator it can take about a minute, but it gets distinctly non-linearly worse as n increases). I have tried to track this down, but very little of the run time seems to be in the f function and Jacobian evaluation, and I think that the likely culprit is probably the linear solver. I have modified the example in the initial post to work with the (slightly) modified new syntax. Where to incorporate the plotting functionality is still uncertain. Currently, a
Currently, the only type of DiffusionReaction(:dX, :X) where DiffusionReactions([(:dX, :X), (:dY, :Y), (:dZ, :Z)]) to create a vector of them directly. If only a single The graph can be a directed on an undirected graph. If it is directed, species only diffuse in the direction of the connection. Internally, if you give an unidrected graph, it is connected to a directed graph with connections in both directions. The initial condition
Parameters are similar, however, contain both spatial and normal parameters. Here, preferably the pair notation is used (listing all parameters in one vector. Alternatively, the parameter can be given as a tuple If an undirected graph with nE edges was given, internally, this is converted to a directed graph with 2*nE edges. Here, if a spatial parameter is given a vector of values of length nE, it is presumed that its n'th value is the values for the two (directed) edges corresponding to the n'th (undirected) edge. |
The format checker fails, but it is the "docs/make.jl" file, but not sure what to do about that. Otherwise this should be all good. |
Can you remind me why you need to use symbols instead of symbolics in specifying spatial reactions? This is a big break from the ModelingToolkit/Catalyst symbolic interface... |
Since the modelingtoolkit/symbolic is never used for those it seemed weird to make them symbolics when I would just internally convert them to symbols/numbers internally. It should be possible to allow them to be both symbols/symbolics though, if that would be desirable? |
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 don't think the Catalyst tests should have timing tests. That is hardware dependent, will likely change when using Github CI. That kind of stuff should go in a SciMLBenchmark instead.
I also think we should switch to taking symbolics for spatial reactions to preserve the interface with how normal reactions are specified, and the more general ModelingToolkit approach of specifying things with symbolics instead of Symbol
s.
Sounds good, |
I have removed the timing tests, and also added the ability to use @parameters dS dI dR
@variables t
@species S(t) I(t) R(t)
SIR_srs_numsym_1 = diffusion_reactions([(:dS, :S), (:dI, :I), (:dR, :R)])
SIR_srs_numsym_2 = diffusion_reactions([(dS, :S), (dI, :I), (dR, :R)])
SIR_srs_numsym_3 = diffusion_reactions([(:dS, S), (:dI, I), (:dR, R)])
SIR_srs_numsym_4 = diffusion_reactions([(dS, S), (dI, I), (dR, R)])
SIR_srs_numsym_5 = diffusion_reactions([(dS, :S), (:dI, I), (dR, :R)])
SIR_srs_numsym_6 = diffusion_reactions([(:dS, :S), (:dI, I), (dR, R)])
u0 = [:S => 990.0, :I => 20.0 * rand_v_vals(small_2d_grid), :R => 0.0]
pV = SIR_p
pE_1 = [:dS => 0.01, :dI => 0.01, :dR => 0.01]
pE_2 = [dS => 0.01, dI => 0.01, dR => 0.01]
pE_3 = [dS => 0.01, :dI => 0.01, :dR => 0.01]
ss_explicit_base = solve(ODEProblem(LatticeReactionSystem(SIR_system, SIR_srs_numsym_1, small_2d_grid), u0, (0.0, 10.0), (pV, pE_1); jac = false), Tsit5()).u[end]
ss_implicit_base = solve(ODEProblem(LatticeReactionSystem(SIR_system, SIR_srs_numsym_1, small_2d_grid), u0, (0.0, 10.0), (pV, pE_1); jac = true), Rosenbrock23()).u[end]
for srs in [
SIR_srs_numsym_1,
SIR_srs_numsym_2,
SIR_srs_numsym_3,
SIR_srs_numsym_4,
SIR_srs_numsym_5,
SIR_srs_numsym_6,
], pE in [pE_1, pE_2, pE_3]
lrs = LatticeReactionSystem(SIR_system, srs, small_2d_grid)
ss_explicit = solve(ODEProblem(lrs, u0, (0.0, 10.0), (pV, pE); jac = false), Tsit5()).u[end]
ss_implicit = solve(ODEProblem(lrs, u0, (0.0, 10.0), (pV, pE); jac = true), Rosenbrock23()).u[end]
@test all(isapprox.(ss_explicit, ss_explicit_base))
@test all(isapprox.(ss_implicit, ss_implicit_base))
end was it something like this you had in mind? |
I'll try to get back for a review later in the week. Sorry for the delay, I'm dealing with a ton of deadlines right now... |
Now worries, let's discuss on Wednesday, probably the quickest. |
I tried to check allocation when using the Jacobian: using Catalyst, OrdinaryDiffEq, BenchmarkTools
SIR_system = @reaction_network begin
α, S + I --> 2I
β, I --> R
end
SIR_p = [:α => 0.1 / 1000, :β => 0.01]
SIR_u0 = [:S => 999.0, :I => 1.0, :R => 0.0]
SIR_sr = diffusion_reactions([(:dS, :S), (:dI, :I), (:dR, :R)])
small_2d_grid = Graphs.grid([5, 5])
SIR_lrs = LatticeReactionSystem(SIR_system, SIR_sr, small_2d_grid)
oprob = ODEProblem(SIR_system, SIR_u0, (0.0, 500.0), SIR_p)
oprob_spat = ODEProblem(SIR_lrs, SIR_u0, (0.0, 500.0), (SIR_p, [:dS=>0.1, :dI=>0.2, :dR=>0.05]))
J_tmp = deepcopy(oprob_spat.f.jac_prototype)
@benchmark oprob_spat.f.jac(J_tmp, oprob_spat.u0, oprob_spat.p, 0.0) It says |
If you merge master into your PR, I capped the formatter script to use the last version before the recent style change. Install that version (1.0.32) and you can format locally, and hopefully it will then only impact files you've modified. |
Ok, this now based on symbolics. That mean it supports stuff like @parameters d1 d2
@variable t
@species X(t)
DiffusionReaction(d1*d2, X) I also tried implementing a spatial f and jacobian for the Brusselator manually, to compare performance. For large system it is almost 2x faster. However, I still get the poor scaling where the simulation time approaches a minute when the number of comaprtments is 10000 (this is without rescaling the diffusion rate as number of compartment are increased, I should note). |
I have recreated the example from https://docs.sciml.ai/JumpProcesses/stable/tutorials/spatial/, but going through a LatticeReactionSystem, will prettify the code and upload tomorrow. |
Have made some improvements to the internals to make it easier to read. This should now also support spatial jump simulations, see #659. I'm fairly happy with this now. What is currently lacking is:
Except for that, I think we just need to create some documentation and plotting interfaces. |
@TorkelE can you put the jump updates in a separate PR to keep this more manageable? Then when this gets merged you can update it to master so it only shows the new commits. |
Done. Tomorrow I will see if someone in the office can help me rebase this on the master branch as well |
The latest master should now be part of this one. The format check still fails' @isaacsas , not sure what is going on though. Either way, when there's opportunity it would be good to deal with this one. If it is possible to make more optimization on the ODE side maybe we could do that later (so that we could move forward with the Jump stuff as well, which depend on this one). |
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.
@TorkelE here are some comments so far. I'm about halfway through the main lattice_reaction_system file, but not sure I'll be able to look at this more today.
Do we have examples of any spatial modeling packages and their notations? It might be helpful in further cleaning up / clarifying the interface here too. |
Do you mean within Julia, or a spatial CRN modelling package? |
I mean a spatial CRN modeling package. Like PyRDME, STEPS, the one that was mentioned in our session at JuliaCon Medyon I think, etc. |
They've all already designed ways to specify lattice reactions, so it might be worth looking at their interface to see if they have nice ways for users to specify the transport model. |
Just checking, if we update it like this, the intended workflow would be something like: using Catalyst, Graphs
rn = @reaction_system begin
(p,d), 0 <--> X
end
@parameters D
@variables t
@species X(t)
srs = [TransportReaction(D, X)]
lattice = Graphs.grid([5, 5])
lrs = LatticeReactionSystem(rn, srs, lattice) ? I would have used |
That is what I added on to my earlier comment. I'd be ok with requiring all species to be declared within the attached @unpack X = rn
@parameters D
@transport_rx $D $X
# or this creates the parameter if not defined
# then in LatticeReactionSystem we need to loop through and
# pull out these parameters from the spatial reactions, as we do in ReactionSystem
@transport_rx D $X We very much should not have users re-declare a species that is already in |
To be honest though, I find the current @species X [transportrates = X]
rs = ...
lrs = LatticeReactionSystem(rs, grid) |
You mean That could work. What if one would want to e.g. create a SIR model, and then from it create different spatial models (where different subsets of species move)? Also, in the future we might want to add spatial reactions that are no just one species moving compartment. It could be a species moving to another compartment while being converted to another one like |
I don't feel strongly about using metadata, so it is fine to keep the |
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
…igin/lattice_reaction_system_may_2023"
Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
e75fda0
to
1f9386d
Compare
Bummer. Still some problem with SteadyStateDiffEq. Avik said that v2 was ready, but they forgot to release it. Should hopefully be out soon, might fix this. |
OK, it have now switched to the latest SteadyStateDiffEq version (v2), but now NonlinearSolve and stuff seem to fail. Saw someone report the same issue on Slack, so might be a common thing? |
A revamped version of #571. While we might be able to tune some stuff, this should work well. Jacobian can be built, so stiff systems can be implemented properly.
Syntax is similar to previously, see this example:
When making a spatial simulation:
u0
is a NxM matrix, where N is the number of species in the (non-spatial) reaction network, and M is the number of compartments.p
is a tupple(pV,pE)
,pV
is a LxM matrix, where L is the number of parameters in the (non-spatial) reaction network, and M is the number of compartments.pE
is a RxS matrix, where R is the number of parameters tied to spatial reactions, and S is the number of connections.Sample code:
Plotting is not added to Catalyst right now, but running this code:
enable us to create a plot through: