-
-
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?
Changes from 7 commits
f2fcae9
674febf
2e4e897
9bb5ff1
8135f72
798ff9c
d98f83f
da9f937
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,6 @@ name: "Tests" | |
|
||
on: | ||
pull_request: | ||
branches: | ||
- master | ||
paths-ignore: | ||
- 'docs/**' | ||
push: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -275,8 +275,7 @@ Notes: | |
the same units, and all reactions have rate laws with units of (species units) / (time | ||
units). Unit checking can be disabled by passing the keyword argument `checks=false`. | ||
""" | ||
struct ReactionSystem{V <: NetworkProperties} <: | ||
MT.AbstractTimeDependentSystem | ||
struct ReactionSystem{V <: NetworkProperties} <: MT.AbstractSystem | ||
"""The equations (reactions and algebraic/differential) defining the system.""" | ||
eqs::Vector{CatalystEqType} | ||
"""The Reactions defining the system. """ | ||
|
@@ -373,7 +372,6 @@ struct ReactionSystem{V <: NetworkProperties} <: | |
(hasnode(is_species_diff, eq.lhs) || hasnode(is_species_diff, eq.rhs)) && | ||
error("An equation ($eq) contains a differential with respect to a species. This is currently not supported. If this is a functionality you require, please raise an issue on the Catalyst GitHub page and we can consider the best way to implement it.") | ||
end | ||
|
||
rs = new{typeof(nps)}( | ||
eqs, rxs, iv, sivs, unknowns, spcs, ps, var_to_name, observed, | ||
name, systems, defaults, connection_type, nps, cls, cevs, | ||
|
@@ -398,7 +396,7 @@ function ReactionSystem(eqs, iv, unknowns, ps; | |
name = nothing, | ||
default_u0 = Dict(), | ||
default_p = Dict(), | ||
defaults = _merge(Dict(default_u0), Dict(default_p)), | ||
defaults = merge(Dict(default_u0), Dict(default_p)), | ||
connection_type = nothing, | ||
checks = true, | ||
networkproperties = nothing, | ||
|
@@ -472,22 +470,24 @@ function ReactionSystem(eqs, iv, unknowns, ps; | |
MT.process_variables!(var_to_name, defaults, unknowns′) | ||
MT.process_variables!(var_to_name, defaults, ps′) | ||
MT.collect_var_to_name!(var_to_name, eq.lhs for eq in observed) | ||
# | ||
|
||
# Computes network properties. | ||
nps = if networkproperties === nothing | ||
NetworkProperties{Int, get_speciestype(iv′, unknowns′, systems)}() | ||
else | ||
networkproperties | ||
end | ||
|
||
# Creates the continuous and discrete callbacks. | ||
ccallbacks = MT.SymbolicContinuousCallbacks(continuous_events) | ||
dcallbacks = MT.SymbolicDiscreteCallbacks(discrete_events) | ||
# Creates the continuous and discrete events. | ||
continuous_events, discrete_events = MT.create_symbolic_events(continuous_events, discrete_events) | ||
|
||
# handles system metadata. | ||
metadata === nothing ? Base.ImmutableDict{Symbol,Any}() : metadata | ||
|
||
ReactionSystem( | ||
eqs′, rxs, iv′, sivs′, unknowns′, spcs, ps′, var_to_name, observed, name, | ||
systems, defaults, connection_type, nps, combinatoric_ratelaws, | ||
ccallbacks, dcallbacks, metadata; checks = checks) | ||
continuous_events, discrete_events, metadata; checks = checks) | ||
end | ||
|
||
# Two-argument constructor (reactions/equations and time variable). | ||
|
@@ -1385,20 +1385,6 @@ function make_empty_network(; iv = DEFAULT_IV, name = gensym(:ReactionSystem)) | |
ReactionSystem(Reaction[], iv, [], []; name = name) | ||
end | ||
|
||
# A helper function used in `flatten`. | ||
function getsubsystypes!(typeset::Set{Type}, sys::T) where {T <: MT.AbstractSystem} | ||
push!(typeset, T) | ||
for subsys in get_systems(sys) | ||
getsubsystypes!(typeset, subsys) | ||
end | ||
typeset | ||
end | ||
|
||
function getsubsystypes(sys) | ||
typeset = Set{Type}() | ||
getsubsystypes!(typeset, sys) | ||
typeset | ||
end | ||
|
||
""" | ||
ModelingToolkit.flatten(rs::ReactionSystem) | ||
|
@@ -1419,11 +1405,10 @@ Notes: | |
function MT.flatten(rs::ReactionSystem; name = nameof(rs)) | ||
isempty(get_systems(rs)) && return rs | ||
|
||
# right now only NonlinearSystems and ODESystems can be handled as subsystems | ||
subsys_types = getsubsystypes(rs) | ||
# right now we only guarantee tht certain types of systems work with flatten | ||
allowed_types = (ReactionSystem, NonlinearSystem, ODESystem) | ||
all(T -> any(T .<: allowed_types), subsys_types) || | ||
error("flattening is currently only supported for subsystems mixing ReactionSystems, NonlinearSystems and ODESystems.") | ||
isnothing(get_systems(rs)) || all(is_allowed_subsystem, get_systems(rs)) || | ||
error("flattening is currently only supported for subsystems mixing ReactionSystems, and Systems withour noise equations and jumps.") | ||
|
||
ReactionSystem(equations(rs), get_iv(rs), unknowns(rs), parameters(rs); | ||
observed = MT.observed(rs), | ||
|
@@ -1438,6 +1423,15 @@ function MT.flatten(rs::ReactionSystem; name = nameof(rs)) | |
metadata = MT.get_metadata(rs)) | ||
end | ||
|
||
# Checks if a system is an allowed subsystem (i.e. no SDE parts and no jump). | ||
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 commentThe 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 commentThe 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 commentThe 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 commentThe 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. |
||
end | ||
# If neither a `ReactionSystem` or a `System`, it is something weird we do not know what it is. | ||
is_allowed_subsystem(sys::MT.AbstractSystem) = false | ||
|
||
function complete_check(sys, method) | ||
if MT.iscomplete(sys) | ||
error("$method with one or more `ReactionSystem`s requires systems to not be marked complete, but system: $(MT.get_name(sys)) is marked complete.") | ||
|
@@ -1611,3 +1605,9 @@ unitless_exp(u) = iscall(u) && (operation(u) == ^) && (arguments(u)[1] == 1) | |
function unitless_symvar(sym) | ||
return (sym isa Symbolics.CallWithMetadata) || (ModelingToolkit.get_unit(sym) == 1) | ||
end | ||
|
||
|
||
### Unsorted ### | ||
|
||
# Function previously used by ModelingToolkit. | ||
MT.refreshed_metadata(::Nothing) = MT.MetadataT() # FIXME: Type piracy |
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