-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Updating parameter/variable values does not update values of dependant defaults #2733
Comments
It wasn't listed as a dependent parameter, just has a default value |
I am not sure I follow? Here, |
The thing is that the default values for parameters encode that they should only initially be that and not that it's a relationship that needs to be enforced. To enforce the relationship one needs to use the parameter dependencies keyword argument as @ChrisRackauckas pointed out. This might be a bit confusing since for variables the defaults refer to an actual constraint and remake seems to treat defaults as enforcing equations, but setp does the correct thing (IMO) and does not update parameters based on defaults. |
Up until now the default value are the value of species/parameters at the beginning of the simulation (after which they can change). The the above example one would expect the initial value of Parameter dependencies is, if I understand it right, a different thing. However, there doesn't seem to be any documentation of it? |
I would expect prob = remake(oprob; u0 = [X => 2.0], use_defaults = true)
@assert prob.ps[d] == 2.0 to do what the original example tries to do. I'm not on my computer to check if it works, though. IIRC, the behavior of |
A clarification on terminology: parameter dependencies and dependent defaults are two different things. Dependent defaults refers to the default value of a variable depending on another variable (this can be an unknown or a parameter, or even an expression involving a mixture of the two). Note that giving Parameter dependencies can be thought of as However, I can see the inconsistency here, where defaults for unknowns are considered when creating an initialization system for the ODEProblem, but ones for parameters aren't. This ties directly in to #2665, which requires allowing parameters in the |
Correct, you need to pass |
I still think there is an issue here that can and should be solved before the creation of the integrator and simulation system initialization. I.e.
Everything else is going to be greatly confusing to users, and seems like asking for more problems down the line (and also cause problems for other packages, like DynamicalSystems, which have some functionality to use |
The thing is in general |
If the user changes |
Consider an example where If I now specify a default for #2665 raises a valid concern where parameters can also be unknowns in the initialization problem. Thus, they will effectively get the same treatment. If |
Right, so I agree that defaults will have effects down the line. However, this was a thing before we started working with initialization and focus on DAEs, I don't see what the introduction of this means that we should throw away old features that people have been utilising. Generally, it is not that defaults permits you to suddenly do something entirely different, e.g. using ModelingToolkit
@variables t X(t)
@parameters p d = X
D = Differential(t)
eqs = [D(X) ~ p - d*X]
@mtkbuild osys = ODESystem(eqs, t)
u0 = [X => 1.0,]
ps = [p => 1.0]
oprob = ODEProblem(osys, u0, 1.0, ps)
oprob[:X] = 2.0 should just be the same as setting using ModelingToolkit
@variables t X(t)
@parameters p d = X
D = Differential(t)
eqs = [D(X) ~ p - d*X]
@mtkbuild osys = ODESystem(eqs, t)
u0 = [X => 2.0,]
ps = [p => 1.0]
oprob = ODEProblem(osys, u0, 1.0, ps) which is hardly controversial. |
Did this work before? Updating |
Well, it was the intended behaviour, there were bugs going around before v9 as well. In this case, we only had the bug being reported this spring. |
@ChrisRackauckas can we get a decision on whether this should be supported? This is currently a blocker on making a Catalyst 14 release with MTK9 support and there seems to be a lot of back and forth in this thread. Right now, and in MTK8 and earlier, it was supported to declare a parameter as a function of unknowns/variables via defaults and have this implicitly mean the parameter was defined in terms of the initial values of the unknowns. It is true that it appears If defining, and consistently updating, parameters that are given by functions of the initial values of unknowns is no longer supported we need to deprecate conservation law elimination in Catalyst 14 and let Catalyst users know it will no longer be supported. Otherwise we are stuck on MTK8 until this is resolved. |
Just to add, if this is something that is supposed to work with |
And modifying @TorkelE's example to use @mtkbuild osys = ODESystem(eqs, t; parameter_dependencies=Dict(d => 2*X))
oprob = ODEProblem(osys, u0, 1.0, ps) gives
|
It should be |
No, it shouldn't be parameter dependencies The way to think about this is, Defaults such as |
But for this case we should be good, right? Let me explain our application Conserved quantities in chemical reaction networksWe have a reaction system with a single thing (X) which can exist in two states (X1 and X2). Basically we have these two reactions:
This creates an ODE with two equations
However, we then make the observation that the quantity X1 + X2 is conserved, i.e. does not change throughout the simulation. If we then create a parameter, Γ, to represent this conserved quantity we get an equation
and representing the species X2 as an observable
(this is a bit similar to what Computing conserved quantities using defaultsNow the only question is, how do we compute Γ? Well, it is a constant
Since The problemSo the problem is that, right now, the value of parameters that have initial conditions are set when a |
Basically, the problem we see here is the same as we see when we have a parametric initial condition:
While the initial example I gave her might be a bit non-intuitive, I think it should be clear why the behaviour in this example is un-desired. |
Maybe I am missing something, but is there a common need for using Test
using ModelingToolkit
using ModelingToolkit: t_nounits as t, D_nounits as D
using DifferentialEquations
@testset begin "all 2x2 combinations of variable <-> parameter defaults"
vars = @variables x1(t) x2(t)
pars = @parameters p3 p4
eqs = [D(x1) ~ 0, D(x2) ~ 0]
@named sys1 = ODESystem(eqs, t, vars, pars; defaults = [x1 => x2*1/2]) # var(var) default
@named sys2 = ODESystem(eqs, t, vars, pars; defaults = [x2 => p3*2/3]) # var(par) default
@named sys3 = ODESystem(eqs, t, vars, pars; defaults = [p3 => x2*3/2]) # par(var) default
@named sys4 = ODESystem(eqs, t, vars, pars; defaults = [p4 => p3*4/3]) # par(par) default
sys1, sys3, sys2, sys4 = complete.([sys1, sys3, sys2, sys4]) # complete all systems
tspan = (0.0, 1.0)
# first, create and initialize all problems
prob1 = ODEProblem(sys1, [ x2 => 2.0], tspan, [p3 => 3.0, p4 => 4.0]) # calculate missing x1 from defaults, works
prob2 = ODEProblem(sys2, [x1 => 1.0, ], tspan, [p3 => 3.0, p4 => 4.0]) # calculate missing x2 from defaults, works
prob3 = ODEProblem(sys3, [x1 => 1.0, x2 => 2.0], tspan, [ p4 => 4.0]) # calculate missing p3 from defaults, works
prob4 = ODEProblem(sys4, [x1 => 1.0, x2 => 2.0], tspan, [p3 => 3.0 ]) # calculate missing p4 from defaults, works
for prob in [prob1, prob2, prob3, prob4]
@test prob[x1] == 1.0 && prob[x2] == 2.0 && prob.ps[p3] == 3.0 && prob.ps[p4] == 4.0
end
# second, update the variables and parameters by switching their signs
prob1 = remake(prob1, u0 = [ x2 => -2.0], p = [p3 => -3.0, p4 => -4.0], use_defaults = true) # works
prob2 = remake(prob2, u0 = [x1 => -1.0, ], p = [p3 => -3.0, p4 => -4.0], use_defaults = true) # fails
prob3 = remake(prob3, u0 = [x1 => -1.0, x2 => -2.0], p = [ p4 => -4.0], use_defaults = true) # works
prob4 = remake(prob4, u0 = [x1 => -1.0, x2 => -2.0], p = [p3 => -3.0 ], use_defaults = true) # works
for prob in [prob1, prob2, prob3, prob4]
@test prob[x1] == -1.0 && prob[x2] == -2.0 && prob.ps[p3] == -3.0 && prob.ps[p4] == -4.0
end
end This all works for me on EDIT: @TorkelE's example just above is also equivalent to the case |
Regarding @TorkelE's example with oprob.ps[X0] = 3.0 # with default X = X0, should now oprob[X] == 3.0? vs. oprob = remake(oprob; p = [X0 => 3.0], u0 = Dict(), use_defaults = true) # with default X = X0, now oprob[X] == 3.0 As a user, I would not expect oprob.ps[X0] = 3.0
oprob.ps[Y0] = 4.0
oprob.ps[Z0] = 5.0
# perhaps many more in a bigger system ... Instead, I think the second method with one call to |
If there are performance involved, I think @hersle makes a good point. To keep Generally, in Catalyst, I think we recommend that |
So there is no problem? If you have a default for
I can see why this is undesirable, but it's undesirable specifically because of the semantics Catalyst wants to enforce. Your concern is that the values in the
As long as you have the default |
Yes. Parameters can be changed during callbacks.
That is a bug, and something I intend to fix eventually. |
Thank you! I did not think of that use, and this clears up all my confusion. For reference, it sounds to me like the TL;DR of this thread is:
|
Yeah. Also an additional point - the relationships during initialization won't be fully respected until #2665 is closed. Specifically, relationships that require parameters to be changed won't work. |
Can this be closed now that parameter initialization is in? |
This doesn't seem to have been fixed though? On 9.47 using Catalyst, SciMLBase
rn = @reaction_network begin
(k1,k2), A <--> B
end
osys = convert(ODESystem, rn; remove_conserved = true)
osys = complete(osys) Which gives a system with
and
remake still doesn't correctly update gamma though: oprob = ODEProblem(osys, [osys.A => 1.0, osys.B => 1.0], (0.0, 1.0), [osys.k1 => 2.0, osys.k2 => 3.0])
oprob2 = remake(oprob; u0 = [osys.B => 2.0])
oprob3 = remake(oprob; u0 = [osys.A => 2.0])
oprob4 = remake(oprob; u0 = [osys.A => 2.0, osys.B => 3.0]) First notice that julia> oprob.p
MTKParameters{Vector{Float64}, Tuple{}, Tuple{}, Tuple{}}([3.0, 2.0, 2.0], (), (), ()) is the same as julia> oprob2.p
MTKParameters{Vector{Float64}, Tuple{}, Tuple{}, Tuple{}}([3.0, 2.0, 2.0], (), (), ()) is the same as julia> oprob3.p
MTKParameters{Vector{Float64}, Tuple{}, Tuple{}, Tuple{}}([3.0, 2.0, 2.0], (), (), ()) is the same as julia> oprob4.p
MTKParameters{Vector{Float64}, Tuple{}, Tuple{}, Tuple{}}([3.0, 2.0, 2.0], (), (), ()) i.e. |
Note I tested here in a new environment with Catalyst master and MTK 9.47.0. |
For a parameter to be solved during initialization, it must satisfy the conditions in https://docs.sciml.ai/ModelingToolkit/dev/tutorials/initialization/#Initialization-of-parameters. Such a parameter will get its final value when
Try |
julia> defaults(osys)
Dict{Any, Any} with 1 entry:
Γ[1] => B(t) + A(t) So the parameter has a default specified but it isn't being used in |
Note the problem here is not the initial problem construction, i.e. |
Thanks, I forgot we need to use |
|
That feels like it shouldn't be required though? Why should a user have to know they have to pass a blank dict? |
Since before |
This |
So what's the solution for Catalyst? Is there a way we can generate an |
Note that in my example above oprob4 = remake(oprob; u0 = [osys.A => 2.0, osys.B => 3.0], p = Dict()) gives julia> oprob4.p
MTKParameters{Vector{Float64}, Tuple{}, Tuple{}, Tuple{}}([3.0, 2.0, 2.0], (), (), ()) so it doesn't appear the |
The solution is parameter initialization. You have a parameter whose value is dependent on the initial conditions of unknowns. Those initial conditions could potentially also be determined by other (algebraic/otherwise) constraints. So you give the parameter a default of When you |
OK, so if Catalyst constructs the ODESystem with |
It seems the problem here is one of the "shortcuts" we take in Note, however, that parameter initialization will not suffer from this issue. Once the integrator is initialized, it will have the correct value of |
I tried passing |
If we defined |
Initialization runs when calling
That's identical to using defaults |
Got it. I would still consider this an open issue then as it appears there is no way to update this parameter value without requiring users to deviate from the standard SciML workflow for updating a problem's u0/p. |
But this is now a standard SciML workflow. SciML exposes new functionality, and MTK codegens to it. |
DAEs and callback have always had an initialization phase before the solve. This is just using the initialization phase to update u0 and p. For DAEs, it's always been the case that u0 would update at the beginning. It is a bit new for this phase to also change p, but it's not new for example for a callback initialization to change p before a solve. |
Also worth noting is that with this initialization, if you |
If I have understood, it sounds like one should no longer assume that |
In general in the past i.e. with |
Since this is considered the desired behavior I will close this again. We will just make a new function in Catalyst, |
I thought this one was reported, but didn't actually find it anywhere. This is both a silent error (not just a crash), and something that we have Catalyst users that report errors with, so would be good to have a fix asap so that we can finally transition Catalyst to the MTK v9.
The text was updated successfully, but these errors were encountered: