SciML / JumpProcesses.jl

Build and simulate jump equations like Gillespie simulations and jump diffusions with constant and state-dependent rates and mix with differential equations and scientific machine learning (SciML)
https://docs.sciml.ai/JumpProcesses/stable/
Other
140 stars 35 forks source link

Tiny fix of `Coevolve()` with the dependency graph check #282

Closed xiaomingfu2013 closed 1 year ago

xiaomingfu2013 commented 1 year ago

this pull request is related to https://github.com/SciML/JumpProcesses.jl/pull/276#issuecomment-1372219257

ChrisRackauckas commented 1 year ago

Can you add a test which catches this? I guess it should be a case with mixed jumps?

xiaomingfu2013 commented 1 year ago

Here is a MWE

using JumpProcesses

maj_rate = [1.]
react_stoich_ = [Vector{Pair{Int, Int}}()]
net_stoich_ = [[1 => 1]]
mass_action_jump_ = MassActionJump(maj_rate, react_stoich_, net_stoich_; scale_rates=false)

affect! = function (integrator)
    integrator.u[1] -= 1
end
cs_rate1(u,p,t) = 0.2 * u[1]
constant_rate_jump = ConstantRateJump(cs_rate1, affect!)
jumpset_ = JumpSet((), (constant_rate_jump,), nothing, mass_action_jump_)

u0 = [0]
tspan = (0.0, 30.0)
prob_ = DiscreteProblem(u0, tspan)
alg = Coevolve()

jprob_ = JumpProblem(dprob_, alg, jumpset_, save_positions=(false, false))

where the JumpSet has one constant rate jump and one mass action jump, but the dependency graph is not provided. Thus, I suppose Coevolve() should show error with https://github.com/SciML/JumpProcesses.jl/blob/2afd12369ef5e848543a9e315d2ab02c8f44a667/src/aggregators/coevolve.jl#L31 But instead, the dependency graph is generated by https://github.com/SciML/JumpProcesses.jl/blob/2afd12369ef5e848543a9e315d2ab02c8f44a667/src/aggregators/coevolve.jl#L33 But Coevolve() will still show error because the length of dependency graph does not fit the total reaction number https://github.com/SciML/JumpProcesses.jl/blob/2afd12369ef5e848543a9e315d2ab02c8f44a667/src/aggregators/coevolve.jl#L45 So at the end, one still needs to provide a correct dependency graph.

ChrisRackauckas commented 1 year ago

Can you add that MWE as a test?

ChrisRackauckas commented 1 year ago

Use Catalyst to generate the problem so it has the dependency graph? Or use one of the hand-built ones and extend it?

isaacsas commented 1 year ago

I just added a test for this based on the MWE.

isaacsas commented 1 year ago

Thanks @palmtree2013

xiaomingfu2013 commented 1 year ago

Thank you for adding the test