Handle different parameters types #797
Conversation
|
This PR is based on #784 (hence the number pf shown commits) |
|
A fix to the symbolic stoichiometries tests (also expanding them a bit with things that are possible now) was added here as well. It basically utilises the ability to designated types for parameters to reenable these tests. |
|
I have reverted the changes for spatial reaction systems, removing the changes to accommodate non-default types. The tests are still there and marked as broken. I will then introduce support for this once the internal remake has been merged. |
TorkelE
force-pushed
the
handle_different_paraemter_types
branch
from
9d793ac
to
37215a9
Compare
There was a problem hiding this comment.
I haven’t reviewed this yet, but please keep in-place test functions in-place. Generally everything related to ODEs and SDEs should be in place unless we are using static arrays, and we definitely shouldn’t be converting to non-recommended workflows (i.el changing existing in place tests to out of place).
| catalyst_jsys = convert(JumpSystem, rs) | ||
| unknownoid = Dict(unknown => i for (i, unknown) in enumerate(unknowns(catalyst_jsys))) | ||
| catalyst_vrj = ModelingToolkit.assemble_vrj(catalyst_jsys, equations(catalyst_jsys)[i], unknownoid) | ||
| @test isapprox(catalyst_vrj.rate(u0_2, ps_2, τ), jumps[i].rate(u0_2, ps_2, τ)) |
There was a problem hiding this comment.
| @test isapprox(catalyst_vrj.rate(u0_2, ps_2, τ), jumps[i].rate(u0_2, ps_2, τ)) | |
| @test isapprox(catalyst_vrj.rate(u0_2, ps_2, τ), jumps[i].rate(u0_2, ps_2, τ)) |
Is ps_2 really the right input for the Catalyst generated function? Doesn't it need something that has been processed into an MTKParameter object now?
There was a problem hiding this comment.
I tried to generate MTKParameters and give as input, but that generates some internal error saying that MTKParameters cannot be used as input. You probably know more about these VariableRateJump than me though, so if you have some suggested update I am happy to make it.
There was a problem hiding this comment.
Should we modify this test then to actually generate the JumpProblem over an ODEProblem and pull the ps and functions to test out of it? Then we don't have to deal with building the functions via the internal MTK functions anymore.
There was a problem hiding this comment.
If you want to leave that for a future PR that is fine -- I'll make note of it in the TODO.
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…atalyst.jl into handle_different_paraemter_types
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
…atalyst.jl into handle_different_paraemter_types
| catalyst_jsys = convert(JumpSystem, rs) | ||
| unknownoid = Dict(unknown => i for (i, unknown) in enumerate(unknowns(catalyst_jsys))) | ||
| catalyst_vrj = ModelingToolkit.assemble_vrj(catalyst_jsys, equations(catalyst_jsys)[i], unknownoid) | ||
| @test isapprox(catalyst_vrj.rate(u0_2, ps_2, τ), jumps[i].rate(u0_2, ps_2, τ)) |
There was a problem hiding this comment.
Should we modify this test then to actually generate the JumpProblem over an ODEProblem and pull the ps and functions to test out of it? Then we don't have to deal with building the functions via the internal MTK functions anymore.
| catalyst_jsys = convert(JumpSystem, rs) | ||
| unknownoid = Dict(unknown => i for (i, unknown) in enumerate(unknowns(catalyst_jsys))) | ||
| catalyst_vrj = ModelingToolkit.assemble_vrj(catalyst_jsys, equations(catalyst_jsys)[i], unknownoid) | ||
| @test isapprox(catalyst_vrj.rate(u0_2, ps_2, τ), jumps[i].rate(u0_2, ps_2, τ)) |
There was a problem hiding this comment.
If you want to leave that for a future PR that is fine -- I'll make note of it in the TODO.
This PR make modifications so that non-default parameter types can be handled. It also adds tests for this in the context of Catalyst.
Currently, this only partially works for LatticeReactionSystems, i.e.
ODEProblems cannot be created if a non-default parameter type has been given. However, the internals orLatticeReactionSystems have been remade a bit in a follow-up PR, so I don't want to touch that to risk messing up the merging of that. Once that is merged, I will ensure this is supported in the spatial case as well.