Catalyst.jl icon indicating copy to clipboard operation
Catalyst.jl copied to clipboard

Fix `merge!` to include the `var_to_name` map

Open lmshk opened this issue 1 year ago • 5 comments

Previously, parameters defined in the second ReactionSystem would not be accessible as properties on the merged system. This also affected @add_reactions.

lmshk avatar Feb 28 '24 15:02 lmshk

Hi, thanks for the PR! We can't merge this at this time as currently any updates to master will break doc builds unfortunately (due to bugs in ModelingToolkit's V8 releases prior to the release of V9). We'll therefore have to wait till Catalyst V14 is merged into master and then we can revisit this.

isaacsas avatar Feb 28 '24 20:02 isaacsas

In the meantime, the following workaround can be used: Instead of @add_reactions system begin ... end, use:

let more =
    @reaction_network begin
        ...
    end
    merge!(system, more)
    merge!(system.var_to_name, more.var_to_name}
    system
end

The second merge! is idempotent and will therefore become a no-op once this PR is merged.

lmshk avatar Mar 03 '24 13:03 lmshk

Happy to hear you found a workaround! I would strongly recommend you change your workflow to creating a second system with any additional reactions and then using ModelingToolkit.extend to merge it into the first. This is the preferred way to merge systems together, and will likely be the only way in Catalyst 14 (since it is the official ModelingToolkit way to merge systems).

isaacsas avatar Mar 03 '24 15:03 isaacsas

See this tutorial: https://docs.sciml.ai/Catalyst/stable/catalyst_functionality/compositional_modeling/#Compositional-modeling-tooling

isaacsas avatar Mar 03 '24 15:03 isaacsas

Sounds like this PR will become obsolete then, feel free to close.

lmshk avatar Mar 04 '24 10:03 lmshk

We've removed these mutating functions for Catalyst V14 so I'm going to close this.

isaacsas avatar May 31 '24 14:05 isaacsas