Fix for issue #1337 - #1338
Merged
Merged
Conversation
From Claude: The C API's Gillespie-averaging entry points - gillespieMeanOnGrid(Ex) and gillespieMeanSDOnGrid(Ex) in wrappers/C/rrc_api.cpp - have two independent bugs, both reproduced below with the S1 -> S2 mass-action model from the issue (k1 = 0.5, S1(0) = 100). Each of these functions runs its own fresh batch of `numberOfSimulations` Gillespie simulations internally (that's the whole point of the numberOfSimulations parameter), so unlike the plain gillespie()/gillespieOnGrid() calls, nothing about them should ever depend on some earlier, unrelated simulation having already run: 1. They used to size their accumulator matrix from RoadRunner::getSimulationData(), which is a default-constructed (0x0, null-backed) matrix until *some* simulation has populated it, and otherwise just holds whatever shape that last simulation happened to have. If nothing had run yet, the very first write into that 0x0 accumulator dereferenced a null pointer and crashed the process - even if the "gillespie" integrator had already been selected. If something unrelated *had* run, but on a different grid, the accumulator was silently the wrong shape. Both are fixed by sizing directly from the requested grid and the model's current selection list, which are known up front and don't require having run anything. 2. In the accumulation loop, every write targeted column 0 unconditionally (`avg(j, 0)`) instead of looping over all columns. Column 0 is time, so only the time column was ever averaged; every species column was left at its zero-initialized value. gillespieMeanSDOnGrid had the same column-0 bug for its variance accumulator, and on top of that never finished the Welford calculation (divide by n-1, sqrt) or copied it into the result - it copied the (broken) mean into Weights instead.
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
1 participant
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.
From Claude:
The C API's Gillespie-averaging entry points - gillespieMeanOnGrid(Ex) and gillespieMeanSDOnGrid(Ex) in wrappers/C/rrc_api.cpp - have two independent bugs, both reproduced below with the S1 -> S2 mass-action model from the issue (k1 = 0.5, S1(0) = 100). Each of these functions runs its own fresh batch of
numberOfSimulationsGillespie simulations internally (that's the whole point of the numberOfSimulations parameter), so unlike the plain gillespie()/gillespieOnGrid() calls, nothing about them should ever depend on some earlier, unrelated simulation having already run:They used to size their accumulator matrix from RoadRunner::getSimulationData(), which is a default-constructed (0x0, null-backed) matrix until some simulation has populated it, and otherwise just holds whatever shape that last simulation happened to have. If nothing had run yet, the very first write into that 0x0 accumulator dereferenced a null pointer and crashed the process - even if the "gillespie" integrator had already been selected. If something unrelated had run, but on a different grid, the accumulator was silently the wrong shape. Both are fixed by sizing directly from the requested grid and the model's current selection list, which are known up front and don't require having run anything.
In the accumulation loop, every write targeted column 0 unconditionally (
avg(j, 0)) instead of looping over all columns. Column 0 is time, so only the time column was ever averaged; every species column was left at its zero-initialized value. gillespieMeanSDOnGrid had the same column-0 bug for its variance accumulator, and on top of that never finished the Welford calculation (divide by n-1, sqrt) or copied it into the result - it copied the (broken) mean into Weights instead.