Skip to content

Reduce symmap_to_varmap usage#1362

Merged
isaacsas merged 12 commits intoversion_16_newfrom
minimise_symmap_to_varmap_usage
Feb 5, 2026
Merged

Reduce symmap_to_varmap usage#1362
isaacsas merged 12 commits intoversion_16_newfrom
minimise_symmap_to_varmap_usage

Conversation

@TorkelE
Copy link
Copy Markdown
Member

@TorkelE TorkelE commented Jan 30, 2026

I've tried to reduce the usage of symmap_to_varmap as much as possible. What I have left is:

  • It is used in quite a few tests. If there are any of the tests that you want to skip/remove symmap_to_varmap from, feel happy to, but I also did not want to intervene in them too indiscriminately.
  • It is used in the HC extension. When we are able to fully move to MTK's HC implementation, we can skip that here, in the meantime, I will let it be. Given that the stability code is fairly related, I am letting that one remain there for now.
  • MTK currently does not do symbol conversion for its MTK problems (I have raised an issue). But since we already have the code here, I will let it remain until that issue is fixed.
  • In the spatial code we, given that we want to store Symbolci internally, but accept symbol inputs, we probably have to let it be a thing for the foreseeable future.

@isaacsas
Copy link
Copy Markdown
Member

isaacsas commented Jan 31, 2026

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.

@TorkelE
Copy link
Copy Markdown
Member Author

TorkelE commented Jan 31, 2026

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)
end

I 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.

@isaacsas
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

@isaacsas isaacsas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you just update this to master and look at the one comment I made this will be good to merge.

Comment thread test/compositional_modelling/component_based_model_creation.jl Outdated
@isaacsas
Copy link
Copy Markdown
Member

isaacsas commented Feb 4, 2026

(The MTK fix has been released now.)

@isaacsas isaacsas mentioned this pull request Feb 4, 2026
43 tasks
isaacsas and others added 11 commits February 4, 2026 21:16
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]>
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]>
@isaacsas isaacsas merged commit a89f447 into version_16_new Feb 5, 2026
16 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants