`Warning: Initialization system is overdetermined` on `remake` cases where it might (might) be redundant

[TorkelE]

Simple question. The first initialisatio nwarning makes sense. However, does the second one? I.e. here it might be straightforward to just update k1's already assigned value, and the observables equation just tracks that? Very possibly this is working as intended, and if so feel happy to close @AayushSabharwal . However, it also felt like a corner case that someone might not have thought about, so figured I should mention it.

using OrdinaryDiffEqDefault, ModelingToolkit
using ModelingToolkit: t_nounits as t, D_nounits as D

# Create model.
@variables X(t) Y(t)
@parameters p d k1 k2 = 0.5
eqs = [
    D(X) ~ p - d*X,
    k1 + Y ~ k2*X
]
@mtkbuild osys = ODESystem(eqs, t)

# Create `ODEProblem`.
u0 = [X => 2.0, Y => 0.5]
pvals = [p => 1.0, d => 0.1, k1 => 0.5]
oprob = ODEProblem(osys, u0, (0.0, 10.0), pvals)

# Try to `remake`.
remake(oprob; p = [k1 => 1.0]) # Warning: Initialization system is overdetermined

[AayushSabharwal]

The second initialization warning is the same as the first. It's not due to k1 being assigned a new value.

[TorkelE]

Right, that was what I thought. In that case it might not make sense here? I.e. if I supply values to k1, k2, ,X, and Y to remake (but which works out), we have situation where there would be an actual problem we'd want to warn about? However, here, the issue (if there was one) has been dealt with in the initial ODEProblem call?

[ChrisRackauckas]

It is entirely possible that this changes the solvability of the equation. Take the system:

Initial(x) ~ 2
k1*x + y ~ 0
Initial(y) ~ 1

We first warn that the system is overdetermined. But yay it's solvable because of accidental happenstance. remake(oprob; p = [k1 => 1.0]), it's no longer a solvable system. Almost certainly the reason is because the new initialization is still overdetermined. That warning is almost certainly pointing you in the right direction to fix your code, which is to probably remove Initial(y) ~ 1.

The main question then is, should we warn once instead of twice? I'm on the fence. If the system worked before remake, and now it didn't... I'm sure the first response is "I didn't change anything! Your solvers aren't robust!" 🤷 if we don't tell people in each instance why their equations unsolvable, they blame us. So in general we've found it's better to be loud about inconsistent systems of equations because if you don't, people just assume everything is buggy rather than checking that 1+1~3 is not an equation our solvers can find an answer to 😅. At least if we warn "this is probably not solvable", and then you get a retcode that said "we couldn't solve it", people are more inclined to say "hmm I maybe did something wrong, let me debug my code before opening an issue".

I guess if we do not change the structural equations of the initialization problem maybe we don't need to warn again. But I'd say making that only print once is a low priority in the grand scheme of things. Repeatedly telling people to fix their code better isn't necessarily a bad thing 😅. In fact, the recommendation some people have been passing around is to just always error by default on overdetermined systems, which isn't necessarily a bad idea either.

[TorkelE]

That makes sense.

Basically, where I was coming from was looking at doing this kind of stuff in Catalyst with the new structural simplification approach. However, in Catalyst we have actively "eliminated" one of the species of the system for the conservation law. The idea was to create a situation where one could then remake the conservation law parameter, and then the eliminated species is free and is adapted as the conservation law parameter changes. But I agree that it is not obvious in MTK with pure structural simplification, as there is less of a notion of which symbolic variables in k1 + Y ~ k2*X are free to adapt to make the equation hold as stuff is updated.

[ChrisRackauckas]

The idea was to create a situation where one could then remake the conservation law parameter, and then the eliminated species is free and is adapted as the conservation law parameter changes

You need to do both. remake(oprob, [y => missing]; p = [k1 => 1.0]) is fine, you're structurally changing the equations and its consistent.

[TorkelE]

Right, so you basically specify the entity that should be free to when one thing in the equation is updated? That makes perfect sense. Feel free to close this from my part, I get what is going on now (or keep open while you decide when to throw what types of warnings and errors).

[ChrisRackauckas]

Yes, we have the missing syntax for removing initial conditions. We probably need better documentation of symbolic remake but that's the flow that was created to make this unambiguous. The choices are that in remake:

1. Always drop any initialization that isn't passed in, i.e. always require that every initial condition and every parameter is passed in.
2. Always interpret remake's symbolic arguments as an update list, i.e. expressions for initial conditions and parameters that didn't exist before are appended to the previous set, and removing conditions has a syntax x => missing.

We went with the latter because it seems nicer for large models: requiring that remake(prob; u0, p) always lists every u0 and p doesn't seem like it would work well in practice since the user could have 2000 variables and oops I forgot one and now I'm underdetermined. The assumption is, you were probably determined before, so you're doing something like now defining a new initial condition and removing one. If you forget to remove one, then you should get a warning that you're now overdetermined.

The double warning here is that in your specific case you were already overdetermined, and then the remake is not changing structure because it's updating an existing condition on k1, and then you're still overdetermined. Warning after remake makes sense because the general case is you will have changed the equations, but in this specific scenario you haven't changed equations so it's a bit redundant. But, because you're overdetermined, that remake could still have made a working case no longer work, so it's not necessarily bad information either: remake(oprob, [y => missing]; p = [k1 => 1.0]) would likely have been a good idea and the warning is still pushing in the right direction.

So while I'm on the fence, I just don't think removing the second warning here is the biggest priority, especially since in most cases it's a good warning. Indeed hyperspecialization of cases is what leads to the bugs seen in Catalyst in the first place: "if you remake and you were overdetermined before and you end up overdetermined again, but the equations are structurally the same, then don't print a warning" just seems like a very easy way to slip a bug in where now we don't print in a case where we wanted to print. So I'd prefer to keep the logic simple unless we really find the complexity absolutely necessary.

[TorkelE]

This makes sense, I agree that one should have to pass full p and u0 vectors to remake.

For now thing works well. i think there might be a solution where Catalyst, simultaneously as a species is eliminated, somehow updates its value to missing in the correct way. That said this is a very minor inconvenience an obscure scenario, so let's not dwell to much on it.

[ChrisRackauckas]

Well for Catalyst you have a determined system because your extra equation is a conservation law with a missing parameter, x1 + x2 ~ gamma where gamma => missing. So if you remake(prob, p = [gamma => 1]), then it is overdetermined and you should remake(prob, u0 = [x2 => missing], p = [gamma => 1]) to say "I now want to set gamma instead of x2". IMO that's clear explicit and intended.

So then you have the ODE case where [x1 => ..., x2 => ...], that's determined, you have the conservation law case, which [x1 => ..., x2 => ...], that's determined, and you have the case where you now want to switch to defining the conservation parameter, ``[x1 => ..., x2 => missing, gamma => ...]`, that's determined. So

simultaneously as a species is eliminated, somehow updates its value to missing in the correct way

My point is that you only need to do that if the user is specifying the conservation parameter.