Skip to content
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

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

Merged
merged 3 commits into from
Jan 5, 2023

Conversation

xiaomingfu2013
Copy link
Contributor

this pull request is related to #276 (comment)

this pull request is related to SciML#276 (comment)
@ChrisRackauckas
Copy link
Member

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

@xiaomingfu2013
Copy link
Contributor Author

xiaomingfu2013 commented Jan 5, 2023

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

error("To use Coevolve a dependency graph between jumps must be supplied.")

But instead, the dependency graph is generated by
dg = make_dependency_graph(length(u), maj)

But Coevolve() will still show error because the length of dependency graph does not fit the total reaction number
error("Number of nodes in the dependency graph must be the same as the number of jumps.")

So at the end, one still needs to provide a correct dependency graph.

@ChrisRackauckas
Copy link
Member

Can you add that MWE as a test?

@ChrisRackauckas
Copy link
Member

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

@isaacsas
Copy link
Member

isaacsas commented Jan 5, 2023

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

Copy link
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GTG if tests pass I think.

@isaacsas isaacsas merged commit da0418e into SciML:master Jan 5, 2023
@isaacsas
Copy link
Member

isaacsas commented Jan 5, 2023

Thanks @palmtree2013

@xiaomingfu2013
Copy link
Contributor Author

Thank you for adding the test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants