Fixes for dependent parameters #2668
Conversation
|
Format or the tests won't run. |
|
oh, sorry, fixed it |
|
I realized that this is not enough, as the parameter dependencies are dropped in several other places. The above 2 are commits are what I got so far, I'm not sure what other places need this. I'll try to add more tests too. |
src/systems/diffeqs/odesystem.jl
Outdated
| @@ -354,6 +354,7 @@ function flatten(sys::ODESystem, noeqs = false) | |||
| get_iv(sys), | |||
| unknowns(sys), | |||
| parameters(sys), | |||
| parameter_dependencies = get_parameter_dependencies(sys), | |||
There was a problem hiding this comment.
We could have parameter dependencies in hierarchical systems, which means this needs the guesses treatment where dependencies from subsystems are namespaced and included in the flattened system
There was a problem hiding this comment.
Isn't the system structurally simplified by the time we get here? 95c8567 would take care of that.
There was a problem hiding this comment.
I changed this to use parameter_dependencies(sys) instead, so now it gets the renamespaced dependencies of the subsystems.
| @testset "getu with parameter deps" begin | ||
| @parameters p1=1.0 p2=1.0 | ||
| @variables x(t) = 0 | ||
|
|
||
| @named sys = ODESystem( | ||
| [D(x) ~ p1 * t + p2], | ||
| t; | ||
| parameter_dependencies = [p2 => 2p1] | ||
| ) | ||
| prob = ODEProblem(complete(sys)) | ||
| get_dep = getu(prob, 2p2) | ||
| @test get_dep(prob) == 4 | ||
| end | ||
|
|
There was a problem hiding this comment.
Could you also add a testcase for hierarchical systems where the subsystems have parameter dependencies? I have a feeling that will fail
There was a problem hiding this comment.
Yeah, it failed. I added a test in db89f68 and I tried to do the renamespacing, but something is off and the dependent parameter does not seem to update correctly.
SebastianM-C
force-pushed
the
obs_dep
branch
2 times, most recently
from
db89f68 to
94428b4
Compare
| if has_parameter_dependencies(sys) && | ||
| (pdeps = get_parameter_dependencies(sys)) !== nothing | ||
| if has_parameter_dependencies(sys) | ||
| pdeps = parameter_dependencies(sys) |
There was a problem hiding this comment.
This is not technically necessary, although it will work regardless. If the system is simplified, all parameter dependencies will be available at the top level. If it isn't simplified, we can't simulate it anyway so an index cache is immaterial.
|
Also needs a rebase, since MTK CI passes now |
SebastianM-C
force-pushed
the
obs_dep
branch
2 times, most recently
from
095c41b to
bc5c51c
Compare
SebastianM-C
force-pushed
the
obs_dep
branch
2 times, most recently
from
84d951a to
c127f24
Compare
|
I can't suggest changes on unchanged files, but in |
This makes dependent parameters available in the observed functions.
…ystem this fixes structural simplification dropping parameter dependencies
Co-authored-by: Aayush Sabharwal <[email protected]>
Co-authored-by: Aayush Sabharwal <[email protected]>
`subsys` might not have parameter dependencies, but subsystems of `subsys` might Co-authored-by: Aayush Sabharwal <[email protected]>
SebastianM-C
changed the title
Co-authored-by: Aayush Sabharwal <[email protected]>
|
Thanks! That seems to have solved the issue. |
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
When
build_explicit_observed_functionwas updated for parameter dependencies, only thepsargument was updated, but internally the function still ckecked for theparametersinstead of thefull_parameters.