-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
Implement system functions (Hill and michaelis-menten) via Symbolics.jl #327
Conversation
The error seems to be due to two systems that should be "identical" no longer are so. This is basically the test we run: using Catalyst
using ModelingToolkit
rn1 = @reaction_network begin
mm(X2,v1,K1), ∅ → X1
mm(X3,v2,K2), ∅ → X2
(k1,k2), X1 ⟷ X3
(k3,k4), X3 + X2 ⟷ X4 + X1
d, (X1,X2,X3,X4) → ∅
end v1 K1 v2 K2 k1 k2 k3 k4 d
@parameters v1 K1 v2 K2 k1 k2 k3 k4 k5 p d t
@variables X1(t) X2(t) X3(t) X4(t) X5(t)
rxs_1 = [Reaction(v1*X2/(K1+X2), nothing, [X1], nothing, [1]),
Reaction(v2*X3/(K2+X3), nothing, [X2], nothing, [1]),
Reaction(k1, [X1], [X3], [1], [1]),
Reaction(k2, [X3], [X1], [1], [1]),
Reaction(k3, [X3,X2], [X4,X1], [1,1], [1,1]),
Reaction(k4, [X4,X1], [X3,X2], [1,1], [1,1]),
Reaction(d, [X1], nothing, [1], nothing),
Reaction(d, [X2], nothing, [1], nothing),
Reaction(d, [X3], nothing, [1], nothing),
Reaction(d, [X4], nothing, [1], nothing)]
rn2 = ReactionSystem(rxs_1 , t, [X1,X2,X3,X4], [v1,K1,v2,K2,k1,k2,k3,k4,d])
rn1.eqs[1].rate # = Catalyst.mm(X2(t), v1, K1)
rn2.eqs[1].rate # = v1*X2(t)*((K1 + X"(t))^-1)
isequal(rn1.eqs[1].rate,rn2.eqs[1].rate) # = false I tried writing rxs_1 = [Reaction(mm(X2,v1,K1), nothing, [X1], nothing, [1]),
Reaction(mm(X3,v2,K2), nothing, [X2], nothing, [1]),
Reaction(k1, [X1], [X3], [1], [1]),
Reaction(k2, [X3], [X1], [1], [1]),
Reaction(k3, [X3,X2], [X4,X1], [1,1], [1,1]),
Reaction(k4, [X4,X1], [X3,X2], [1,1], [1,1]),
Reaction(d, [X1], nothing, [1], nothing),
Reaction(d, [X2], nothing, [1], nothing),
Reaction(d, [X3], nothing, [1], nothing),
Reaction(d, [X4], nothing, [1], nothing)] but this yields a I'm trying to remove this test and rerun. |
Looks good to me! We can always add back those other names if someone complains by defining them too. |
|
I'm thinking about removing that test (since as you say, it maybe just doesn't make sense anymore). In a different file we do test whenever two such systems evaluate to similar things: ### Fetch required packages and reaction networks ###
using DiffEqBase, Catalyst, Random, Test
using ModelingToolkit: get_states, get_ps
using StableRNGs
rng = StableRNG(12345)
### Tests various cutom made functions ###
new_hill(x, v, k, n) = v*x^n/(k^n+x^n)
new_poly(x,p1,p2) = .5*p1*x^2
new_exp(x,p) = exp(-p*x)
custom_function_network_1 = @reaction_network begin
hill(X,v1,K1,2), X + Y --> Z1
mm(X,v2,K2), X + Y --> Z2
p1*X^2+p2, X + Y --> Z3
exp(-p3*Y), X + Y --> Z4
hillR(X,v3,K3,2), X + Y --> Z5
mmR(X,v4,K4), X + Y --> Z6
end v1 K1 v2 K2 p1 p2 p3 v3 K3 v4 K4
custom_function_network_2 = @reaction_network begin
new_hill(X,v1,K1,2), X + Y --> Z1
v2*X/(X+K2), X + Y --> Z2
2*new_poly(X,p1,p2)+p2, X + Y --> Z3
new_exp(Y,p3), X + Y --> Z4
v3*(K3^2)/(K3^2+X^2), X + Y --> Z5
v4*K4/(X+K4), X + Y --> Z6
end v1 K1 v2 K2 p1 p2 p3 v3 K3 v4 K4
f1 = ODEFunction(convert(ODESystem,custom_function_network_1),jac=true)
f2 = ODEFunction(convert(ODESystem,custom_function_network_2),jac=true)
g1 = SDEFunction(convert(SDESystem,custom_function_network_1))
g2 = SDEFunction(convert(SDESystem,custom_function_network_2))
for factor in [1e-2, 1e-1, 1e0, 1e1, 1e2, 1e3]
u0 = factor*rand(rng,length(get_states(custom_function_network_1)))
p = factor*rand(rng,length(get_ps(custom_function_network_2)))
t = rand(rng)
@test all(abs.(f1(u0,p,t) .- f2(u0,p,t)) .< 100*eps())
@test all(abs.(f1.jac(u0,p,t) .- f2.jac(u0,p,t)) .< 100*eps())
@test all(abs.(g1(u0,p,t) .- g2(u0,p,t)) .< 100*eps())
end This mostly worked, but there were occasional small errors, especially for larger input (I think especially having the exponent 2 in hill functions can amplify small numerical differences). I modified this file by:
|
Ok, I switched out the network which which didn't make sense to another (so we no longer test if the expression mm(...) is equal to v*X...). |
test/custom_functions.jl
Outdated
@test all(abs.(f1.jac(u0,p,t) .- f2.jac(u0,p,t)) .< 100*eps()) | ||
@test all(abs.(g1(u0,p,t) .- g2(u0,p,t)) .< 100*eps()) | ||
end | ||
@test all(abs.(f1(u0,p,t) .- f2(u0,p,t)) .< 10e-6) |
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.
Why such a loose tolerance now?
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.
Some test fails when I do 1e-8
. I'm quite sure that it's not the functions that are written wrongly (they pas lots of these tests), but that it is something which happens with the computer when it calculates large numbers. Don't actually know much about how this works, but looking into creating a minimal example.
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.
Ok, turns out by making each reaction depend on their own X
and Y
value, things become a bit better (somehow the errors compounded in these variables across all reactions). Still can only push it to 1e-10
reliably, but it is still much better.
Probably should bump the ModelingToolkit version. The error is likely due to needing to regenerate the Latexify tests... |
Turned out Latexify used some of the previous functions to detect and insert hill functions etc. That's fixed now, and things should (hopefully) run fine now. There's a still a problem when using equation form: using Catalyst
using Latexify
rn = @reaction_network begin
mm(X,v,K), 0 --> X
end v K
latexify(convert(ODESystem, rn)) which doesn't unfold the |
Ok, just running latexify in a notebook, things look fine to me. However, there seems to be some difference in the actual codes used for comparison in the tests, e.g. r = @reaction_network begin
hillR(X2,v1,K1,n1)*hill(X4,v1,K1,n1), ∅ → X1
hill(X5,v2,K2,n2), ∅ → X2
hill(X3,v3,K3,n3), ∅ → X3
hillR(X1,v4,K4,n4), ∅ → X4
hill(X2,v5,K5,n5), ∅ → X5
(k1,k2), X2 ⟷ X1 + 2X4
(k3,k4), X4 ⟷ X3
(k5,k6), 3X5 + X1 ⟷ X2
(d1,d2,d3,d4,d5), (X1,X2,X3,X4,X5) ⟶ ∅
end v1 K1 n1 v2 K2 n2 v3 K3 n3 v4 K4 n4 v5 K5 n5 k1 k2 k3 k4 k5 k6 d1 d2 d3 d4 d5
# Latexify.@generate_test latexify(r)
@test latexify(r) == replace(
raw"\begin{align}
\require{mhchem}
\ce{ \varnothing &->[K1^{n1} v1^{2} \left( \mathrm{X4}\left( t \right) \right)^{n1} \mathrm{inv}\left( K1^{n1} + \left( \mathrm{X2}\left( t \right) \right)^{n1} \right) \mathrm{inv}\left( K1^{n1} + \left( \mathrm{X4}\left( t \right) \right)^{n1} \right)] X1}\\
\ce{ \varnothing &->[v2 \left( \mathrm{X5}\left( t \right) \right)^{n2} \mathrm{inv}\left( K2^{n2} + \left( \mathrm{X5}\left( t \right) \right)^{n2} \right)] X2}\\
\ce{ \varnothing &->[v3 \left( \mathrm{X3}\left( t \right) \right)^{n3} \mathrm{inv}\left( K3^{n3} + \left( \mathrm{X3}\left( t \right) \right)^{n3} \right)] X3}\\
\ce{ \varnothing &->[v4 K4^{n4} \mathrm{inv}\left( K4^{n4} + \left( \mathrm{X1}\left( t \right) \right)^{n4} \right)] X4}\\
\ce{ \varnothing &->[v5 \left( \mathrm{X2}\left( t \right) \right)^{n5} \mathrm{inv}\left( K5^{n5} + \left( \mathrm{X2}\left( t \right) \right)^{n5} \right)] X5}\\
\ce{ X2 &<=>[k1][k2] X1 + 2 X4}\\
\ce{ X4 &<=>[k3][k4] X3}\\
\ce{ 3 X5 + X1 &<=>[k5][k6] X2}\\
\ce{ X1 &->[d1] \varnothing}\\
\ce{ X2 &->[d2] \varnothing}\\
\ce{ X3 &->[d3] \varnothing}\\
\ce{ X4 &->[d4] \varnothing}\\
\ce{ X5 &->[d5] \varnothing}
\end{align}
", "\r\n"=>"\n") errors. The actual text now produced by
(haven't gotten @korsbo would you have time to have a look at this sometime in the near future? I've been unable to display the old text in Jupyetr/LaTeX without errors, so not sure exactly what is going on. We could just replace the old text with the new one (but it might be useful to have someone actually understand what is happening). Alternatively we can comment out the test, or put this PR on hold until you can check that we don't introduce something bad. |
@TorkelE I don't know if there are other problems but what you write in the last comment seems to be fine but that you just need to generate new tests. For Once you have that the |
That's great! I regenerated the tests and just pasted them into the doc, they now pass locally on my computer. |
Added some additional tests, hoping to avoid the large drop in coverage. After this it should be good to go. |
Looks great, thanks! |
Previously, when the function
recursive_expand_functions!()
scanned the input expressions, it searched for a couple of keywords (like:hill
), and if encountering an appropriate function call, replacing it with the correct expression (from a manually written list). This PR will:There are now 5 functions specially implemented:
mm
,Mm
, andMM
.mmR
,MmR
, andMMR
.hill
andHill
.hillR
andHillR
.hillAR
,hillC
,HillAR
, andHillC
.The keywords are the recognised calls for the respective function.
The implementation looks like this (example for the Hill function):
(I used Wolfram Alpha to double check the derivatives)
It should note that this is currently a breaking change. Previously we allowed some more obscure function calls to be recognized (these were recognised for the repressing hill:
([:hill_repressor, :hillr, :hillR, :HillR, :hR, :hR, :Hr, :HR, :HILLR])
). Initially I thought this change might implement these as normal function in people's environment, and figured it might be good to be conservative. I removed some options.I don't think anyone uses these. But it would be possible to implement these using Symbolics, or as a compromise, implement them using the old methods. However, this might also be a breaking change we could accept to entirely scrap the old system.