-
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
Conversation
src/reactionsystem.jl
Outdated
end | ||
|
||
""" ODESystem constructor """ | ||
function ModelingToolkit.ODESystem(sbmlfile::String) |
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.
eventually need to fix the kwargs, so that @named mymodel = ODESystem(sbmlfile)
will 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.
where can I find documentation of the @named
macro. I've never used 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.
help?> @named
Rewrite @named y = foo(x) to y = foo(x; name=:y).
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 you basically just need to change this to
function ModelingToolkit.ODESystem(sbmlfile::String; kwargs...)
and then pass them to the actual constructor
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.
Thanks!
src/reactionsystem.jl
Outdated
|
||
""" Expand reversible reactions to two reactions """ | ||
function expand_reversible(model) | ||
model # Todo: convert all Reactions that are `reversible=true` to a forward and reverse reaction with `reversible=false`. |
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.
Do we need to do this if X + Y ↔ XY
is valid catalyst? https://catalyst.sciml.ai/stable/tutorials/basics/#Combining-several-reactions-in-one-line
Alternatively, it could be something that Catalyst needs. It seems like a useful transform to have.
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 worth pinging Sam about
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 way as SBML would describe the kineticLaw for a reversible reaction is X + Y ↔ XY
is kAs*X*Y-kDi*XY
. Catalyst however needs kAs*X*Y, kDi*XY
, that is two kineticLaws. I mean this is a simple example, but somehow we need to find a way that generally works in splitting a combined kineticLaw in a forward and reverse kineticLaw. Any suggestions how to do this in a clean way?
So I finally got to reading this, quick comments:
Yeah, I'm not sure that others would really welcome pulling in the extra dependencies. Also, the code is very ODE specific.
Does this mean that there are cases that reaction has 2 different math rules that get triggered by changing the reaction direction?
I'll keep it at |
Yeah, I just thought for developing purposes it is easier if it is in the same repo. But can be moved now or whenever we feel like things have stabilised. What do you think?
Not quite, if I understand you correctly. It just means that that you have a single math rule for the reaction rate which can be positive or negative, depending on whether the forward or reverse reaction is dominating. Consider the reaction
I think SBML.jl should be agnostic of MTK (in the long run). It would make more sense to convert to Symbolics.jl symbols, I believe. |
For an XML example of bidirectional reaction you can look at #39 |
Yeah to that very last point. You can quickly see how MathML does this lowering here https://github.com/SciML/MathML.jl/blob/main/src/maps.jl. Basically I just string match the name of the first element of the "apply" XML node and return the equivalent Julia function |
Why was this closed? |
it was probably automatically closed when I merged #39 ... |
@fbergmann or @skeating: When an SBML reaction is reversible, it still only contains a single |
src/reactionsystem.jl
Outdated
end | ||
subsdict = _get_substitutions(model) | ||
# PL: Todo: @Anand: can you convert kinetic_math to Symbolic expression. Perhaps it would actually better if kinetic Math would be a Symbolics.jl expression rather than of type `Math`? But Mirek wants `Math`, I think. | ||
kl = substitute(reaction.kinetic_math, subsdict) # PL: Todo: might need conversion of kinetic_math to Symbolic MTK expression |
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.
IIRC @exaexa was taking this Math* sublanguage conversion. But I can get started on it for sure
… is nothing but throw warning.
…simulation results of pure ReactionSystem will be therefore be incorrect
""" 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 """ | ||
function ModelingToolkit.ODEProblem(sbmlfile::String,tspan;kwargs...) # PL: Todo: add u0 and parameters argument | ||
odesys = ODESystem(sbmlfile) | ||
ODEProblem(odesys, [], tspan; kwargs...) | ||
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 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 comment
The 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.
This is now partly merged and partly going to https://github.com/paulflang/SBMLToolkit.jl . Thanks! |
reactionsystem.jl
should probably eventially be moved to theModelingToolkit.jl
package (to get rid of all the ModelingToolkit/Catalyst/Symbolics dependencies here). Other things to clarify:kineticLaws
extensive (see SBML.jl and SciML integration #31 )kineticLaws
from Reactions wherereversible=true
.Reaction.kinetic_math
. This will allow to convert it to the right MTK symbols.