-
Notifications
You must be signed in to change notification settings - Fork 12
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
draft Model
to MTK.___System conversion.
#40
Changes from 65 commits
cfe431e
fd6ad63
32f5e4d
5f94736
d188def
402acbc
6558506
de7d16b
bf6b080
f837186
f9221d1
09a7228
8e6a03c
7e58ca6
4c7e8dd
2c483c0
3347462
b235622
4da59f3
f7af82d
6b64f41
fb84639
a3dce09
570bc7c
beda38e
ec1fa52
6ee8876
bfe4170
40b1d98
ce4f03b
537296b
5455600
e686e91
6913135
a1af25a
1ffe96a
dce53c3
44d88f5
69fa2ff
3d0e09f
667240f
2d98356
42fcd07
d56ab94
b7d9abb
f528a2f
1909a19
3da6440
1202b30
a9a19af
f23b0c1
77a5743
e51201d
778ba4a
48db58b
3600346
9932b3b
7e11c2e
d714fb5
ea55421
b11f448
4169c47
87766de
f460328
59fd135
f726cdb
3c9d313
67a8f84
bbe19eb
4ae0332
1070601
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 |
---|---|---|
@@ -1,15 +1,25 @@ | ||
name = "SBML" | ||
uuid = "e5567a89-2604-4b09-9718-f5f78e97c3bb" | ||
authors = ["Mirek Kratochvil <[email protected]>", "LCSB R3 team <[email protected]>"] | ||
version = "0.4.0" | ||
version = "0.4.1" | ||
|
||
[deps] | ||
Catalyst = "479239e8-5488-4da2-87a7-35f2df7eef83" | ||
IfElse = "615f187c-cbe4-4ef1-ba3b-2fcf58d6d173" | ||
Libdl = "8f399da3-3557-5675-b5ff-fb832c97cbdb" | ||
ModelingToolkit = "961ee093-0014-501f-94e3-6117800e7a78" | ||
Pkg = "44cfe95a-1eb2-52ea-b672-e2afdf69b78f" | ||
SBML_jll = "bb12108a-f4ef-5f88-8ef3-0b33ff7017f1" | ||
SHA = "ea8e919c-243c-51af-8825-aaa63cd721ce" | ||
SparseArrays = "2f01184e-e22b-5df5-ae63-d93ebab69eaf" | ||
Symbolics = "0c5d862f-8b57-4792-8d23-62f2024744c7" | ||
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" | ||
|
||
[compat] | ||
julia = "1" | ||
|
||
[extras] | ||
OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed" | ||
|
||
[targets] | ||
test = ["OrdinaryDiffEq"] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,224 @@ | ||
""" ReactionSystem constructor """ | ||
function ModelingToolkit.ReactionSystem(model::Model; kwargs...) # Todo: requires unique parameters (i.e. SBML must have been imported with localParameter promotion in libSBML) | ||
checksupport(model) | ||
model = make_extensive(model) | ||
rxs = mtk_reactions(model) | ||
species = [] | ||
for k in keys(model.species) | ||
push!(species, create_var(k,Catalyst.DEFAULT_IV)) | ||
end | ||
params = vcat([create_param(k) for k in keys(model.parameters)], [create_param(k) for (k,v) in model.compartments if !isnothing(v.size)]) | ||
ReactionSystem(rxs,Catalyst.DEFAULT_IV,species,params; kwargs...) | ||
end | ||
|
||
""" ReactionSystem constructor """ | ||
function ModelingToolkit.ReactionSystem(sbmlfile::String; kwargs...) | ||
conversion_options = Dict("promoteLocalParameters" => nothing, | ||
"expandFunctionDefinitions" => nothing, | ||
"expandInitialAssignments" => nothing) | ||
model = readSBML(sbmlfile;conversion_options=conversion_options) | ||
ReactionSystem(model; kwargs...) | ||
end | ||
|
||
""" ODESystem constructor """ | ||
function ModelingToolkit.ODESystem(model::Model; kwargs...) | ||
rs = ReactionSystem(model; kwargs...) | ||
model = make_extensive(model) # PL: consider making `make_extensive!` to avoid duplicate calling in ReactionSystem and here | ||
u0map = get_u0(model) | ||
parammap = get_paramap(model) | ||
defaults = Dict(vcat(u0map, parammap)) | ||
convert(ODESystem, rs, defaults=defaults) | ||
end | ||
|
||
""" ODESystem constructor """ | ||
function ModelingToolkit.ODESystem(sbmlfile::String; kwargs...) | ||
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. same for this dispatch, I think this is type-piracy https://docs.julialang.org/en/v1/manual/style-guide/#Avoid-type-piracy. It's not a big deal ATM though, so I'm fine 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. Not a problem now, I think:
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 real question is where reaction network importers like our SBML importer (but also CellML and BioNetGen importers) should go in the end. I am not yet convinced that having separate packages for every model specification language is the right way to go. What type of file is handled can be automatically inferred from the ending or header given a filename as String.
@isaacsas: any thoughts? 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. @shahriariravanian: what are your plans for CellMLToolkit. Do you think it should be moved under ReactionNetworkImporters.jl (see also Sam's and Chris' comment below)? Or shall it remain a standalone library? 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 eventually it is a good idea to put all of them under the same package, but ReactionNetworkImporters doesn't seem like the right place for CellMLToolkit. First, CellMLToolkit mainly outputs ODESystems (and possibly DAESystems in the next version), not reaction networks. While CellMLToolkit specification can handle reaction networks, very few actual CellML files contain one (the same that SBML files can define an ODE, but most don't). For whatever historical reason, CellML and SBML have diverged to their own niches. Second, while it is currently only an importer, the future plan for CellMLToolkit is -- after adding DAEs and implementing units handling and Unitful integration -- to write an exporter. I don't know if you plan to add export functionality to SBML, but I feel that to use the full benefits of symbolic manipulation, we need to have the capability to output the results. 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. @ChrisRackauckas @isaacsas @shahriariravanian @anandijain : After @shahriariravanian 's response that CellMLToolkit will grow bigger and does not fit well under the ReactionNetworkImporters umbrella I am inclined to have all Toolkits for biomodel importers as independent packages. Most users have their favourite model specification language and will ever only need one importer, so there are few reasons to combine them in one package. The only reason I can think of, is to make sure they have a similar API. But for the few users who use two or more model specification languages we can still take care that the API looks similar across importers. To this end, I could imagine mimicking the constructors of the
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. Please don't shadow, just extend. 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. To avoid misunderstandings, I assume shadowing is creating another function with the same name that takes precedence over the old function. And extending means overloading with a more specific type to make use of multiple dispatch. Is this correct? Anyway, I do not get it to work. Especially not without avoiding type piracy.
Do you have any suggestions? |
||
model = readSBML(sbmlfile) | ||
ODESystem(model; kwargs...) | ||
end | ||
|
||
""" ODEProblem constructor """ | ||
function ModelingToolkit.ODEProblem(model::Model,tspan;kwargs...) # PL: Todo: add u0 and parameters argument | ||
odesys = ODESystem(model) | ||
ODEProblem(odesys, [], tspan; kwargs...) | ||
end | ||
|
||
""" ODEProblem constructor """ | ||
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. regarding the docstrings, please include full function types and signatures, and preferably some hint about what precisely the function does it & how. |
||
function ModelingToolkit.ODEProblem(sbmlfile::String,tspan;kwargs...) # PL: Todo: add u0 and parameters argument | ||
odesys = ODESystem(sbmlfile) | ||
ODEProblem(odesys, [], tspan; kwargs...) | ||
end | ||
Comment on lines
+36
to
+46
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 would assume it would be best to have the user explicitly construct the ODESystem -> ODEProblem. This is one of those shorthands that causes doc problems. 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. Looks fishy for sure, and from the user perspective the code with the shorthand is almost as long as without the shorthand. |
||
|
||
""" Check if conversion to ReactionSystem is possible """ | ||
function checksupport(model::Model) | ||
for s in values(model.species) | ||
if s.boundary_condition | ||
@warn "Species $(s.name) has `boundaryCondition` or is `constant`. This will lead to wrong results when simulating the `ReactionSystem`." | ||
end | ||
end | ||
end | ||
|
||
""" Convert intensive to extensive expressions """ | ||
function make_extensive(model) | ||
model = to_initial_amounts(model) | ||
model = to_extensive_math!(model) | ||
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. is this an inplace modification (implied by !) or a normal new-value-returning function? |
||
model | ||
end | ||
|
||
""" Convert initial_concentration to initial_amount """ | ||
function to_initial_amounts(model::Model) | ||
model = deepcopy(model) | ||
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. you don't need a full copy here, just copy()ing the model struct and deepcopying the species is okay. |
||
for specie in values(model.species) | ||
if isnothing(specie.initial_amount) | ||
compartment = model.compartments[specie.compartment] | ||
if !isnothing(compartment.size) | ||
specie.initial_amount = (specie.initial_concentration[1] * compartment.size, "") | ||
else | ||
@warn "Compartment $(compartment.name) has no `size`. Cannot calculate `initial_amount` of Species $(specie.name). Setting `initial_amount` to `initial_concentration`." | ||
specie.initial_amount = (specie.initial_concentration[1], "") | ||
end | ||
specie.initial_concentration = nothing | ||
end | ||
end | ||
model | ||
end | ||
|
||
""" Convert intensive to extensive mathematical expression """ | ||
function to_extensive_math!(model::SBML.Model) | ||
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 find the intensive vs. extensive naming a bit confusing, but that's maybe because I didn't work with this much. If I get it correctly, this here means basically "substitute the compartment sizes" right? 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. Yes it is confusing. Even the SBML specs confused it (should be fixed now or soon). But it has a clear definition in physics and I cannot think of a better terminology. I am open to alternative suggestions, 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. What does it really mean then? "Relative to volume of the container?" |
||
function conv(x::SBML.MathApply) | ||
SBML.MathApply(x.fn, SBML.Math[conv(x) for x in x.args]) | ||
end | ||
function conv(x::SBML.MathIdent) | ||
x_new = x | ||
if x.id in keys(model.species) | ||
specie = model.species[x.id] | ||
if !specie.only_substance_units | ||
compartment = model.compartments[specie.compartment] | ||
if isnothing(compartment.size) | ||
@warn "Specie $(x.id) hasOnlySubstanceUnits but its compartment $(compartment.name) has no size. Cannot auto-correct the rate laws $(x.id) is involved in. Please check manually." | ||
else | ||
x_new = SBML.MathApply("*", SBML.Math[ | ||
SBML.MathVal(compartment.size), | ||
x]) | ||
specie.only_substance_units = true | ||
end | ||
end | ||
end | ||
x_new | ||
end | ||
conv(x::SBML.MathVal) = x | ||
conv(x::SBML.MathLambda) = | ||
throw(DomainError(x, "can't translate lambdas to extensive units")) | ||
for reaction in values(model.reactions) | ||
reaction.kinetic_math = conv(reaction.kinetic_math) | ||
end | ||
model | ||
end | ||
|
||
""" Get dictonary to change types in kineticLaw """ | ||
function _get_substitutions(model) | ||
subsdict = Dict() | ||
for k in keys(model.species) | ||
push!(subsdict, Pair(create_var(k),create_var(k,Catalyst.DEFAULT_IV))) | ||
end | ||
for k in keys(model.parameters) | ||
push!(subsdict, Pair(create_var(k),create_param(k))) | ||
end | ||
for k in keys(model.compartments) | ||
push!(subsdict, Pair(create_var(k),create_param(k))) | ||
end | ||
subsdict | ||
end | ||
|
||
""" Convert SBML.Reaction to MTK.Reaction """ | ||
function mtk_reactions(model::Model) | ||
subsdict = _get_substitutions(model) | ||
rxs = [] | ||
if length(model.reactions) == 0 | ||
throw(ErrorException("Model contains no reactions.")) | ||
end | ||
for reaction in values(model.reactions) | ||
reactants = Num[] | ||
rstoich = Num[] | ||
products = Num[] | ||
pstoich = Num[] | ||
for (k,v) in reaction.stoichiometry | ||
if v < 0 | ||
push!(reactants, create_var(k,Catalyst.DEFAULT_IV)) | ||
push!(rstoich, -v) | ||
elseif v > 0 | ||
push!(products, create_var(k,Catalyst.DEFAULT_IV)) | ||
push!(pstoich, v) | ||
else | ||
@error("Stoichiometry of $k must be non-zero") | ||
end | ||
end | ||
if (length(reactants)==0) reactants = nothing; rstoich = nothing end | ||
if (length(products)==0) products = nothing; pstoich = nothing end | ||
symbolic_math = convert(Num, reaction.kinetic_math) | ||
|
||
if reaction.reversible | ||
symbolic_math = getunidirectionalcomponents(symbolic_math) | ||
kl = [substitute(x, subsdict) for x in symbolic_math] | ||
push!(rxs, ModelingToolkit.Reaction(kl[1],reactants,products,rstoich,pstoich;only_use_rate=true)) | ||
push!(rxs, ModelingToolkit.Reaction(kl[2],products,reactants,pstoich,rstoich;only_use_rate=true)) | ||
else | ||
kl = substitute(symbolic_math, subsdict) | ||
push!(rxs, ModelingToolkit.Reaction(kl,reactants,products,rstoich,pstoich;only_use_rate=true)) | ||
end | ||
end | ||
rxs | ||
end | ||
|
||
""" Infer forward and reverse components of bidirectional kineticLaw """ | ||
function getunidirectionalcomponents(bidirectional_math) | ||
err = "Cannot separate bidirectional kineticLaw `$bidirectional_math` to forward and reverse part. Please make reaction irreversible or rearrange kineticLaw to the form `term1 - term2`." | ||
bidirectional_math = Symbolics.tosymbol(bidirectional_math) | ||
bidirectional_math = simplify(bidirectional_math; expand=true) | ||
if SymbolicUtils.operation(bidirectional_math) != + | ||
throw(ErrorException(err)) | ||
end | ||
terms = SymbolicUtils.arguments(bidirectional_math) | ||
fw_terms = [] | ||
rv_terms = [] | ||
for term in terms | ||
if (term isa SymbolicUtils.Mul) && (term.coeff < 0) | ||
push!(rv_terms, Num(-term)) # PL: @Anand: Perhaps we should to create_var(term) or so? | ||
else | ||
push!(fw_terms, Num(term)) # PL: @Anand: Perhaps we should to create_var(term) or so? | ||
end | ||
end | ||
if (length(fw_terms) != 1) || (length(rv_terms) != 1) | ||
throw(ErrorException(err)) | ||
end | ||
return (fw_terms[1], rv_terms[1]) | ||
end | ||
|
||
""" Extract u0map from Model """ | ||
function get_u0(model) | ||
u0map = [] | ||
for (k,v) in model.species | ||
push!(u0map,Pair(create_var(k,Catalyst.DEFAULT_IV), v.initial_amount[1])) | ||
end | ||
u0map | ||
end | ||
|
||
""" Extract paramap from Model """ | ||
function get_paramap(model) | ||
paramap = Pair{Num, Float64}[] | ||
for (k,v) in model.parameters | ||
push!(paramap,Pair(create_param(k),v)) | ||
end | ||
for (k,v) in model.compartments | ||
if !isnothing(v.size) | ||
push!(paramap,Pair(create_param(k),v.size)) | ||
end | ||
end | ||
paramap | ||
end | ||
|
||
create_var(x, iv) = Num(Variable{Symbolics.FnType{Tuple{Any},Real}}(Symbol(x)))(iv).val | ||
create_var(x) = Num(Variable(Symbol(x))).val | ||
function create_param(x) | ||
p = Sym{Real}(Symbol(x)) | ||
ModelingToolkit.toparam(p) | ||
end |
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'm not sure we will want to steal the String dispatch, since there are other formats that create a ReactionSystem (BioNetGen IIRC).
I think just having
ModelingToolkit.ReactionSystem(model::Model; kwargs...)
might be good enough.It's two function calls, but saves us from committing type piracy down the line.