WIP: Revamp MassActionJump initialization#561
Open
isaacsas wants to merge 4 commits intoSciML:v10_workingfrom
Open
WIP: Revamp MassActionJump initialization#561isaacsas wants to merge 4 commits intoSciML:v10_workingfrom
isaacsas wants to merge 4 commits intoSciML:v10_workingfrom
Conversation
…structs Move mutable rate working-copy (maj_rates) out of MassActionJump and into all 10 aggregation structs + RxRates. Rates are now lazily materialized via fill_scaled_rates! during initialize! instead of eagerly in the JumpProblem constructor. This fixes aliasing bugs where multiple JumpProblems sharing the same MAJ could silently corrupt each others rates. Key changes: - Add maj_rates field to all aggregation structs and RxRates - Add fill_scaled_rates! dispatcher for parameterized vs explicit MAJs - Update evalrxrate to 4-arg signature taking explicit maj_rates - Propagate maj_rates through calculate_jump_rate, rejectrx, bracketing - Remove update_parameters!, finalize_parameters_hook! - Simplify reset_aggregated_jumps! (remove update_jump_params kwarg) - JumpProblem constructor keeps original parameterized MAJ - Add merge guards for mapper-backed MAJs - Add SpatialMassActionJump guard for parameterized MAJs - Remove dead 1-arg mapper callables - Update all tests for lazy rate materialization pattern Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Narrow MAJ merge guards to only block custom (non-MassActionJumpParamMapper) mapper types. Built-in parameterized MAJ merges via varargs and JumpSet vectors work correctly and are tested in ssa_callback_test.jl. - Fix 3-arg evalrxrate call in test/spatial/reaction_rates.jl to use eval_massaction_rate which handles both MassActionJump and SpatialMassActionJump correctly. - Add fill_scaled_rates! call before spatial rate tests to populate maj_rates. - Update scale_rates_field_test.jl merge test to verify built-in merges succeed and custom mapper merges error. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- setup_majump_to_merge now copies arrays before creating the merge accumulator, preventing in-place mutation from corrupting the original MAJ net_stoch/reactant_stoch arrays - massaction_jump_combine(::Nothing, ::MassActionJump) now creates a safe copy via setup_majump_to_merge instead of returning the original MAJ - Remove scale_rates and useiszero as JumpProblem kwargs; they are now properties of the MassActionJump itself. Passing them raises an informative ArgumentError Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Member
Author
Pre-merge checklistBefore merging this PR, the following temporary changes need to be reverted:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Immutable MassActionJump Refactor: Move Working Rates to Aggregation Structs
Motivation
MassActionJump.scaled_rateswas mutated in-place byupdate_parameters!(called fromremake,reset_aggregated_jumps!, andfinalize_parameters_hook!). This caused aliasing bugs where multipleJumpProblems sharing the same MAJ silently corrupted each other's rates, and a class of stale-rate bugs when parameters changed at runtime.Design
Move the mutable rate working-copy out of
MassActionJumpinto a newmaj_rates::Vector{T}field on all aggregation structs. Rate recomputation now happens lazily duringinitialize!via a newfill_scaled_rates!dispatcher, eliminatingupdate_parameters!entirely.Key design points:
scaled_rates === nothing):fill_scaled_rates!callsmaj.param_mapper(dest, maj, params)— the mapper owns all scaling logicscaled_rates <: AbstractVector):fill_scaled_rates!copies the immutablescaled_ratesintomaj_rates{Nothing}MAJ unchangedinitialize!(triggered byinit,solve, orreset_aggregated_jumps!), always reflecting current parametersChanges
Core infrastructure:
(mapper)(dest, maj, params)toMassActionJumpParamMapperfill_scaled_rates!dispatcher that branches on MAJ type parameterevalrxrateto 4-arg signature(speciesvec, rxidx, majump, maj_rates)taking an explicit rates vectormaj_ratesthroughcalculate_jump_rate,rejectrx, andget_majump_bracketsAggregation structs:
maj_rates::Vector{T}field to all 10 aggregation structs (Direct, FRM, DirectCR, SortingDirect, NRM, CCNRM, RSSA, RSSACR, RDirect, Coevolve)maj_rates::Vector{F}field to spatialRxRatesfill_scaled_rates!call to allinitialize!methods and spatialfill_rates_and_get_times!Problem-level simplifications:
using_paramsbranch from both JumpProblem constructors (main + PureLeaping) — store original MAJ directlyreset_aggregated_jumps!— removeupdate_jump_paramskwarg (throws informative error if passed)update_parameters!calls fromremakefinalize_parameters_hook!entirelyMassActionJumpParamMapperSafety guards:
massaction_jump_combineandJumpSetvector constructor)SpatialMassActionJumpfrom a parameterized MAJrescale_rates_on_updatemismatch check on MAJ mergesTests:
scale_rates_field_test.jl: new tests for mapper callable API, immutability after remake, merge error guardsjprob_symbol_indexing.jl: all rate checks go throughdiscrete_jump_aggregation.maj_ratesafterinitssa_callback_test.jl:scale_ratestest uses MAJ-level kwargregular_jumps.jl: PureLeaping test usesfill_scaled_rates!directlyMTK Compatibility
MTKBase's
JumpSysMajParamMapperneeds to add one method to integrate with this change:Dependencies:
SciML/SciMLBase.jl#1252 and SciMLBase release
JumpProcesses #557