-
-
Notifications
You must be signed in to change notification settings - Fork 219
Cases of creation of NonlinearSystem
s have been broken
#3411
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
Comments
Supporting |
Actually just the creation of the nlprob = NonlinearProblem(nlsys, u0, ps);
|
That's what I mentioned
It can't solve for |
I thought the idea was that the observed equations could be used to solve for This all works for ODEs, right, so there is some part of the update where we lost NonlinearSystem support? |
For time-dependent systems, yes. However, running initialization to solve for |
Right, so practically, how should we handle this in Catalyst? I.e. what kind of information should we/should not provide? Do correct me if I am wrong, but at least the cases of this that we had in our tests suite in Catalyst used to work without problems. |
Yeah this would have worked for some time, but it was incredibly broken and thus we hotfixed it out. For time-independent systems, initialization only solves for parameters using parameter dependencies and |
Well, I am glad stuff got fixed at least! Do you have any idea of when this might be available again? |
I can't promise a timeline, unfortunately. I'm aware of the issue, but am currently focused on fixing a plethora of CI issues across SciML. |
So should we make a separate issue for the changes discussed in the last few comments here that mean we can't use conservation law elimination with NonlinearSystems? That is blocker for making a new Catalyst release. |
What's the issue? |
This issue (basically observables and defaults in |
From above:
To solve for the values of conserved constants we need to use initial conditions. Previously, MTK used |
No the point that is being made is that equation was omitted. I assume that what was meant was: eqs = [
0 ~ k1 * (Γ[1] - X1) - k2 * X1,
0 ~ k1 * (Γ[2] - Y1) - k2 * Y1,
0 ~ 1 - V,
0 ~ 1 - W,
]
initialization_eqs = [
Γ[1] ~ X1 + X2
Γ[2] ~ Y1 + Y2
]
@mtkbuild osys = ODESystem(eqs, t, [X1, X2, Y1, Y2, V, W], [k1, k2, V0, v, w, Γ]; initialization_eqs) ? See, this is exactly why the initial values are parameters.
Since the initial values are parameters, |
Just checking, have Also, when I try to construct the system this way, indexing seems broken for e.g. eqs = [
0 ~ k1 * (Γ[1] - X1) - k2 * X1,
0 ~ k1 * (Γ[2] - Y1) - k2 * Y1,
0 ~ 1 - V,
0 ~ 1 - W,
]
initialization_eqs = [
Γ[1] ~ X1 + X2
Γ[2] ~ Y1 + Y2
]
@mtkbuild nlsys = NonlinearSystem(eqs, [X1, X2, Y1, Y2, V, W], [k1, k2, V0, v, w, Γ]; initialization_eqs)
u0 = [X1 => 1.0, X2 => 2.0, Y1 => 3.0, Y2 => 4.0, V => 5.0, W => 6.0]
ps = [k1 => 0.1, k2 => 0.2, V0 => 3.0]
nlprob = NonlinearProblem(nlsys, u0, ps);
nlprob[X1] # 0.0, should be 1.0. |
That doesn't happen right now, which is what @TorkelE is referring to. The reason is that it's a little annoying to do since we can't just include all the observed equations. We need the ones that only depend on variables whose value is given as a constant and even then I'm not sure it's always valid. If the user has an observed equation y ~ 2x
z ~ p - y and defaults |
|
|
No. The system has no unknowns - it is linearly solvable by MTK. So any inital guesses you provide don't hold because MTK analytically solved the system. |
Sounds good. But in that case, why can I no longer declare observables here as I have previously done? I.e. previously we have just made
This is a bit confusing no? I.e. in this case MTK solves my nonlinear problem already when I create it, meaning that the guesses are not stored as values. But in other cases the problem is solved when I make the Finally some other weird stuff. If you do nlsol = solve(nlprob, NewtonRaphson()) then you got nlsol[X1] # -0.0 (not 0.0) like, it is the same thing, so it doesn't matter, but still feels a bit weird. Also, using this approach I can no longer evaluate nlprob[X2] # ERROR: ArgumentError: Symbol X2(t) is not present in the system. (same for the solution |
@ChrisRackauckas Just to add on to what @TorkelE is saying. In limited testing I have found that passing |
What's confusing? You gave a linear system to MTK, MTK solved it for you. Why would you want to provide guesses for something that's linearly solvable? If you don't want MTK to simplify this (for whatever reason) don't call
This is because you provide the equation for |
Yes. If the |
Well two things though. (1) we of course aren't deprecating it, but (2) we purposefully do not document the
Good point. So then what about letting the user saying it explicitly? Since that's what's wanted here, and we now have the grammar for it: eqs = [
0 ~ k1 * (Γ[1] - X1) - k2 * X1,
0 ~ k1 * (Γ[2] - Y1) - k2 * Y1,
0 ~ 1 - V,
0 ~ 1 - W,
]
initialization_eqs = [
Γ[1] ~ Initial(X1) + Initial(X2)
Γ[2] ~ Initial(Y1) + Initial(Y2)
]
@mtkbuild nlsys = NonlinearSystem(eqs, [X1, X2, Y1, Y2, V, W], [k1, k2, V0, v, w, Γ]; initialization_eqs) For the nonlinear system, the Initial(X1) is the guess. So this is saying, use the guesses to define the constant Γ and then solve the system, which IIUC is what's wanted. I haven't checked, but this seems reasonable to support and would be the right way for Catalyst to specify what it wants? |
|
This is a bit of a contrived case though. We're talking about a DSL being able to identify the equations it needs to use to solve for the parameters it wants to solve for. Is this something easily identifiable in general? |
The reason I ask is that while the proposed syntax works it might not be feasible longer term. If later on we figure out a way to identify the observed equations to use they might conflict with the user-provided equations and lead to a falsely overdetermined system (because equation equality is a superficial check) |
|
The issue is not using |
MTK will eliminate those laws.
As I illustrated earlier, it's not whether they're solvable, but how to choose the equations to solve them from. |
Seems like no conservation law was detected or removed to me. |
@AayushSabharwal You are right in your initial comment about |
There is a whole lot to unpack here:
Look, I know Catalyst relied on some internals in the v5 period. But let's not use that as justification that we made a breaking change mid v9. I understand where you're coming from, Catalyst mostly learned a very older version of MTK and incrementally moved, and some things didn't move when it should've, but that doesn't mean that things just randomly changed. You're talking about things from 4-5 years ago and 4-5 breaking updates ago. Maybe it doesn't feel like that long ago but the history is all shown there, it really was that long ago. Catalyst now feels like MTK is unstable because these incremental changes means it's using things weirdly/incorrectly. Some things just happened to work in special cases when using internals. But... let's not rely on that. Let's do things right. Let's get Catalyst up to date so it's using the actual public APIs from v8 forwards and everything will feel much more stable! Isn't that what we all want?
You didn't add the equation. The whole point is that the system doesn't guess things and solves exactly the mathematical description provided.
No observables are "disabled"
Okay yes, it's under-conditioned. The complete mathematical description of the system is: using ModelingToolkit, OrdinaryDiffEq, Test
using ModelingToolkit: t_nounits as t, D_nounits as D
# Defines the model.
@parameters k1 k2 V0
@variables X1(t) X2(t) Y1(t) Y2(t) V(t) = V0 W(t)
@parameters v = V w = W Γ[1:2] = missing [guess = [1.0, 1.0]]
eqs = [
D(X1) ~ k1 * (Γ[1] - X1) - k2 * X1,
D(X2)~ k1 * (Γ[2] - Y1) - k2 * Y1,
D(Y1) ~ 1 - V,
D(Y2) ~ 1 - W,
Γ[1] ~ X1 + X2
Γ[2] ~ Y1 + Y2
]
initialization_eqs = [
Γ[1] ~ Initial(X1) + Initial(X2)
Γ[2] ~ Initial(Y1) + Initial(Y2)
]
@mtkbuild osys = ODESystem(eqs, t, [X1, X2, Y1, Y2, V, W], [k1, k2, V0, v, w, Γ]; initialization_eqs) where then eqs = [
D(X1) ~ k1 * (Γ[1] - X1) - k2 * X1,
D(X2)~ k1 * (Γ[2] - Y1) - k2 * Y1,
D(Y1) ~ 1 - V,
D(Y2) ~ 1 - W,
Γ[1] ~ X1 + X2
Γ[2] ~ Y1 + Y2
]
initialization_eqs = [
Γ[1] ~ Initial(X1) + Initial(X2)
Γ[2] ~ Initial(Y1) + Initial(Y2)
]
@mtkbuild osys = NonlinearSystem(eqs, t, [X1, X2, Y1, Y2, V, W], [k1, k2, V0, v, w, Γ]; initialization_eqs) Is there anything ambiguous about that? |
Observables is something MTK figures out. It's not something you specify to the system directly. All equations that are true about your model during the entire integration should be provided as the equations of the system. MTK is capable of figuring out which ones to make observed.
No, the equations shouldn't be moved directly to
Consider the case when we just have this one initialization equation in the nonlinear system G ~ Initial(X) + Initial(Y) Where |
This is why the observed equations are in the initialization system's construction. So if it's not possible then the system is unsatisfiable.
Yes.
If it's possible then we do, yes. It's just a system of equations. We write out the whole list and solve it if possible, or show it's not satisfiable. |
Yeah but the entire reason for this syntax is we can't include all observed variables in the initialization system of |
Why not? All of those equations have to hold at steady state as well. |
But not necessarily for the initial guess, right? If we include all observed of the ODE as initial equations in the Nonlinear system we'll very likely end up with an InitialFailure |
So, my understanding is that you are requesting that we change from directly inputting the eliminated quantities as observables, we instead add the conservation law equations, creating a DAE, and then letting Even if this was something that technically was not meant to be accessible, I still think it would be nice to support normal observables that can be used, especially for how these work really effortlessly with stochastic systems (i.e. while I do not know about non-CRN modelling software, this is something that at least in the CRN field all other tools support). This is something that basically works already (with the exception of some cases like |
SDEs are handled the same way. You just add Brownian variables https://docs.sciml.ai/ModelingToolkit/stable/tutorials/stochastic_diffeq/
We still need to talk about what to do on Levy processes in general, but I would think we can make that just work the same with a jump variable. In the current reaction setup, we could have structural simplify just removes the equalities from |
Letting structural simplification do this is much easier in the sense of making it support different noise types. That's the reason for the Brownian form.
What would be the advantage? |
Is there actually any theory on what happens with the structural simplification algorithms when applied to SDEs? It also doesn't really work with Catalyst, as those SDEs depend on creating a Also, is then plans to just disable conservation laws for JumpSystems until further notice? I think if that is the case, we would rather just stay in our current MTK version where conservation laws work, and then once there is a tearing implementation for Jumps we will look at transfering Catalyst to the latest MTK version. |
Yes. https://www.sciencedirect.com/science/article/pii/S0377042703010239 justifies the mass matrix index 1 DAE solvers in StochasticDiffEq and https://arxiv.org/abs/2102.00605 justifies the index reduction algorithm. Note that we specifically avoid what's required to build the counterexample by not allowing noise terms in the algebraic equations.
Uhh what? There is at least no regression from previous versions.... where is this even coming from? What is this even referencing? |
Just to clarify, we don't allow conserved species removal for jump models since it would probably be less computationally efficient (doing it for the chemical master equation does make sense). |
Sam is right, I got int confussed, I meant observables generally.
It this is basically just reduction of SADEs? I.e. pure SDEs won't be touched?
Maybe I overreacted, if observables still is going to work like they are currently I agree there is no major problem. It would prefer to handle these uniformly at least across Catalyst. I.e. until |
Yes.
And just to be clear here, as Sam says, jump models don't use structural reduction because it's not necessarily helpful. With ODEs, enforcing x^2 + y^2 = 1 by defining y = sqrt(1 - x^2) can help, emphasis on can because of dummy derivative singularities etc. see the whole JuliaCon workshop on that. But with jump processes, you have discrete updates, +1, -1, and because of that by design you have conserved quantities in the solver. So you don't get a numerical accuracy improvement by removing them. Though in theory in some cases you could subset your model by eliminating some variables, that's just a minor performance thing. |
Right, that makes sense, got it. I think that is one reason I think it would be useful to keep the option of general observables open, although I realise it might not been as trivial to integrate with the remaining system as I had hoped. |
This used to work, but doesn't anymore:
The text was updated successfully, but these errors were encountered: