-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
Bifurcation kit extension #707
Conversation
aa8caab
to
2065fb4
Compare
Functionality is there (just need to add an error check), needs tests and docs. Requires SciML/ModelingToolkit.jl#2337 to merge to handle cases with conservation laws |
The required MTK PR have now been merged. However, I think we still would need to wait for a new release to take advantage of it. |
@@ -1456,7 +1461,7 @@ function Base.convert(::Type{<:JumpSystem}, rs::ReactionSystem; name = nameof(rs | |||
JumpSystem(eqs, get_iv(flatrs), sts, ps; | |||
observed = MT.observed(flatrs), | |||
name, | |||
defaults = MT.defaults(flatrs), | |||
defaults = _merge(defaults,MT.defaults(flatrs)), |
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.
Does merging after flattening give the correct namespacing of parameters and variables? This seems like it is going to cause problems. Merging probably needs to happen before flattening.
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.
Not, that is a good point. I just used the current order of things, but if you suggest changing than that is be better.
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.
Can you add a test to check this?
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.
You mean providing defaults to a convert
for a system with sub-systems, which got defaults?
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.
Yes
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.
Can you check this works with coupled ODEs and add a corresponding test? (Do we want to allow coupled algebraic equations here too? If not, we probably need a check that any coupled equations are just ODEs.)
We cannot actually couple to ODEs: #727 I will couple to another ReactionSystem, should be enough. |
1098f48
to
3f313c1
Compare
This should be ready now. Unfortnatley the CI seems bugged out, keep giving stuff like:
in various places, and I have no idea why. |
CI works on my computer, but keeps failing for reasons here. I the PR is done. I think if I rebase on the right stuff it should be good, just tell me when we can merge this one and I will rebase and it should hopefully work. |
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.
Let's get this working with composed systems too. I think it should just require manually flattening in the BifurcationProblem
before you start making checks and calculating conservation laws.
4e40780
to
6ebd743
Compare
f20d62a
to
f624106
Compare
@TorkelE what is the status on this? I see doc build failures currently? |
this is ready, think the doc failure has appeared recently though. I will rebase on master and if docs still fail fixing it should be relatively straightforward. |
6ebd743
to
3406b7c
Compare
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
3406b7c
to
aa0d28a
Compare
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Enables BifurcationKit
BifurcationProblem
s to be created fromReactionSystem
s.Example workflow: