-
-
Notifications
You must be signed in to change notification settings - Fork 80
[WIP] v16 changes to get tests passing #1322
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: version_16
Are you sure you want to change the base?
Conversation
is_allowed_subsystem(sys::ReactionSystem) = true | ||
function is_allowed_subsystem(sys::System) | ||
return (isnothing(MT.get_noise_eqs(sys)) || isempty(MT.get_noise_eqs(sys))) && | ||
(isnothing(MT.get_jumps(sys)) || isempty(MT.get_jumps(sys))) |
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.
We should support non-reaction jumps now (and Brownians). They should be straightforward to just merge with the reaction jumps when converting later.
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.
(But probably makes sense to save those for follow-ups after you get the current functionality working here…)
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.
Yeah, I was going to give this one (and some other composability stuff) to you to re-do as you see best, right now I just try to make as much as possible pass as soon as possible.
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.
Whatever code I write here likely requires going through in tons of details to make it actually good.
Co-authored-by: Sam Isaacson <[email protected]>
@@ -62,7 +62,7 @@ LaTeXStrings = "1.3.0" | |||
Latexify = "0.16.6" | |||
MacroTools = "0.5.5" | |||
Makie = "0.22.1" | |||
ModelingToolkit = "9.73" | |||
ModelingToolkit = "9.73, 10" |
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.
We should drop MTK < 10. I don't think Catalyst will work with both after these changes.
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.
agree
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 know if we should invest time in keeping this updated. I think the MTK one is now supposed to support finding all steady-states (but I haven't played with it).
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.
If the MTK extension is good enough we can just update the docs to show its use.
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 haven't seen the latest version, but if/when the MTK version is good enough, we should default to that one instead (however, last I heard it only found one, unsure if that has changed though).
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 does but it isn't really documented in MTK. See the tests though: https://github.com/SciML/ModelingToolkit.jl/blob/master/test/extensions/homotopy_continuation.jl
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.
Maybe just ignore updating our extension for now. Once we have core working with V10 we can see if the MTK extension is good enough for us and, if so, update the docs / drop the extension. If not, we update the extension at that time.
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 HC ones are actually working locally, should be fine, the only one that I might need some work on is the BifKit one (the other ones are all good locally, so should be little effort to get working). But yes, will focus on the core functionality stuff first.
Extensions are working lcoally, so will have to check why it fails here. Next, I will try to go through failing tests until we have something where everything pretty much works. Once we are there, there are a decent chunk of going through the code and make it actually respectable... |
Ok, "everything" passes now. Next, I will go through the individual pieces one by one to revert stuff that is now marked as |
Very much work in progress.