Reduce symmap_to_varmap usage#1362
Conversation
|
Unfortunately we still need it for jumps. Let’s hold off on this as I have a user Brownian and jumps WIP PR that works locally, and I don’t want to have to deal with rebasing since it also makes changes around symmap. I may also try to put in a fix for the jump issue, but I’m still testing if what I have is reasonable. |
|
Ok, got it. A lot of things actually seems passing though. E.g. for the Hybrid case the only error is a failure to create the problem in this one # Checks that various model options (observables, events, defaults and metadata, differential equations,
# non-default_iv) works for hybrid models.
let
# Creates the model (X species is pure jump, Y is pure ODE, and Z1,Z2 are mixed).
# Hybrid species have non-constant rates containing the two other species.
rn = @reaction_network begin
@ivs τ
@differentials Δ = Differential(τ)
@parameters X0 Y0
@species X(τ) = X0 [description = "A jump only species"] Y(τ) = Y0 [description = "An ODE only species"]
@variables Ztot(τ) = 0.0 # Default needed for MTK v10+ callback compilation
@observables Ztot ~ Z1 + Z2
@discrete_events [1.0] => [Z1 ~ Pre(Z1) + 1.0]
@continuous_events [Y ~ 1.0] => [Y ~ 5.0]
@equations Δ(V) ~ Z1 + X^2 - V
(p,d), 0 <--> X
d, Y --> 0, [physical_scale = PhysicalScale.ODE]
(k*X, k*Y), Z1 <--> Z2, ([physical_scale = PhysicalScale.ODE], [physical_scale = PhysicalScale.Jump])
end
# Simulates the model.
u0 = [:Z1 => 1.0, :Z2 => 2.0, :V => 1.5]
ps = [:p => 2.0, :d => 0.5, :k => 1.0, :X0 => 0.1, :Y0 => 4.0]
prob = HybridProblem(rn, u0, (0.0, 10.0), ps)
sol = solve(prob, Tsit5(); tstops = [1.0 - eps(), 1.0 + eps()])
# Tests that the model contain the correct stuff.
@test ModelingToolkitBase.getdescription(rn.X) == "A jump only species"
@test ModelingToolkitBase.getdescription(rn.Y) == "An ODE only species"
@test sol[:X][1] == sol.ps[:X0] == 0.1
@test sol[:Y][1] == sol.ps[:Y0] == 4.0
@test sol[:V][1] == 1.5
@test sol[:Ztot] ≈ sol[rn.Z1 + rn.Z2]
@test minimum(sol[:Y]) ≈ 1.0
@test maximum(sol[:Y]) ≈ 5.0 atol = 1e-1 rtol = 1e-1
@test all(isequal([rn.τ], Symbolics.arguments(Symbolics.value(u))) for u in unknowns(rn))
@test sol(1.0 - eps(); idxs = :Z1) + 1 ≈ sol(1.0 + eps(); idxs = :Z1)
endI don;t think there error is related, but probably something obscure in MTK, but would take some time to investigate. Happy to hold of merging until later though. |
|
That specific test is the MTK bug I was mentioning. It is due to MTK not properly handling the symbols that are registered as belonging to the generated affect system for the events (the code to handle that was added at some point to ODE and SDE problems but not jump problems). I have a fix for it I will PR to MTK tomorrow. Then we can hopefully fully drop symmap_to_varmap in our conversion functions. |
isaacsas
left a comment
There was a problem hiding this comment.
I think if you just update this to master and look at the one comment I made this will be good to merge.
|
(The MTK fix has been released now.) |
Version 16 new
Add comprehensive tests to verify save_positions keyword correctly controls jump saving behavior for previously untested scenarios: - ConstantRateJump (Hill/MM kinetics): Verify saveat gives exact times with save_positions=(false,false) for non-mass-action constant-rate jumps - SDE+MassActionJump: Test save_positions forwarding in hybrid SDE+Jump systems - SDE+VariableRateJump: Test ContinuousCallback save_positions in SDE+VRJ hybrids Also tighten existing VRJ tests to verify exact saveat times instead of loose bounds. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Uncomment and restructure tests for solution and integrator indexing which now work correctly with HybridProblem - Fix test logic: separate mutation (=) from assertion (==) - Fix typo: :dD should be :kD - Reorder tests: solution tests before integrator mutation (shared state) - Move broken remake test comment to end of block with clear explanation - Only remake with Symbol/Dict u0 remains broken (upstream MTKBase issue) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The .planning folder should not be committed to the repository. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add save_positions tests for ConstantRateJump and SDE+Jump systems
Symbol keys can be passed directly to ODEProblem using the merged Dict form, so symmap_to_varmap conversion is not needed here. Co-Authored-By: Claude Opus 4.5 <[email protected]>
Hybrid tests remake
Since symmap_to_varmap is no longer exported from Catalyst, the extension modules need to use the fully qualified name. Co-Authored-By: Claude Opus 4.5 <[email protected]>
I've tried to reduce the usage of
symmap_to_varmapas much as possible. What I have left is:symmap_to_varmapfrom, feel happy to, but I also did not want to intervene in them too indiscriminately.