Do not apply the reaction center symmetry factor under TotalRate - #92
Open
wshlavacek wants to merge 1 commit into
Open
Do not apply the reaction center symmetry factor under TotalRate#92wshlavacek wants to merge 1 commit into
wshlavacek wants to merge 1 commit into
Conversation
FunctionalRxnClass::update_a scales the propensity by baseRate. For that class baseRate holds the reaction center symmetry factor and nothing else, because the class is built with baseRate set to 1 and never calls setBaseRate. It applied that factor whether or not the rule uses TotalRate, so a TotalRate rule with a symmetric reaction center ran at a fraction of the rate the model asks for. For a homodimer the fraction is one half. TotalRate means the rate law gives the whole propensity of the rule. The symmetry factor corrects a counting problem, namely that a symmetric pattern matches the same reaction more than once. Under TotalRate there is no counting to correct, because the rate is stated outright, so the factor should not be applied. Measured on the model in issue RuleWorld#90, a homodimer stating a total rate of 0.02 over 1000 time units starting from 400 free monomers. About 20 reactions should fire, each consuming two monomers, which leaves about 360. master 383.4 with this change 361.6 v1.14.3 360.3 The check has to sit where the propensity is computed. NFinput calls setTotalRateFlag well after both the ReactionClass constructor and setBaseRate, so the flag is still false at both of those points and a check placed there would never fire. Only FunctionalRxnClass needs the change. BioNetGen forces every TotalRate rate law into a function even when the rate is a bare constant, so a TotalRate rule never reaches the elementary path where setBaseRate applies the factor. BioNetGen also rejects TotalRate on Sat, MM and Hill rate laws, on Arrhenius, and on local functions. BasicRxnClass is left alone. It reads the folded baseRate in its own TotalRate branch, but BioNetGen cannot produce that combination, and matching v1.14.3 for hand written XML is more useful than an unreachable correction. test/symmetry/symmetry_factor_total_rate.bngl and its XML put a symmetric and an asymmetric TotalRate pool on the same expected survivor count, 2000 out of 4000. The asymmetric pool has a symmetry factor of 1, so no change to the factor can move it. validate.py gains a matching test next to the existing symmetry tests. Checked both ways against a build of master. The new test fails at 2990.8 without the change and passes at about 2000 with it, and the existing symmetry tests pass either way. This came in with "Apply the reaction center symmetry factor on every rate law, not just Ele" from PR RuleWorld#89. That change was correct for ordinary rate laws. The TotalRate case looks like it was missed because the same member is shared.
wshlavacek
added a commit
to lanl/bngsim
that referenced
this pull request
Aug 23, 2026
Five days and 38 commits since 0.14.0 (2026-08-18), across 36 changelog entries. The batch is dominated by the mratio work (#451, #453, #456, #457), the clock and switch machinery (#421, #436, #443, #445, #465, #473, #475), the emitters that turn a rate law into compilable C (#437, #448, #460, #464, #467, #470), and a re-vendor of RuleMonkey from 3.9.0 to 3.10.0. MAJOR bump, not MINOR. MINOR is reserved for additive changes and bug fixes that do not change observable behaviour, and three classes of entry fail that test. New refusals, meaning a run that returned a number now raises. mratio refuses the arguments its continued fraction cannot be trusted with (#453). It used to return answers wrong by a factor of eighteen, and in one measured case with the wrong sign, saying nothing. Issue #456 then narrowed that refusal by adding an asymptotic expansion that certifies its own accuracy, but what is left is still a refusal where there used to be a number. Separately, the vendored RuleMonkey now refuses TotalRate rules for the one shape where it and NFsim genuinely disagree, rather than picking a reading silently. Numbers that move because they were wrong before. NFsim no longer applies the reaction centre symmetry factor under TotalRate (#426), which had been running a TotalRate rule with a symmetric reaction centre at half the rate the model asks for. A BNGL rate law that switches on simulation time or on a counter species is no longer integrated straight through the switch (#443, #445), so trajectories through such a switch change. And a RuleMonkey rate law built on reactant_N() had a propensity of zero, so any rule using one never fired at all. Additions that ask nothing of a downstream pin. capabilities() answers behavioural questions and names the resolved build (#431). A model calling mratio() in a rate law gets an analytic forward sensitivity instead of the difference quotient (#457). A Simulator now says whether its gradient is analytic and why not, and that reason survives the codegen cache (#438). Downstream pins should read the mratio refusal and the two TotalRate entries before upgrading. Everything else either fixes a wrong answer or adds a surface. Not included. RuleWorld/nfsim#92 is still open, so the NFsim carry queue is unchanged and issue #428 stays open to track dropping that carry once it lands.
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.
Fixes #90.
FunctionalRxnClass::update_a scales the propensity by baseRate. For that class
baseRate holds the reaction center symmetry factor and nothing else, because the
class is built with baseRate set to 1 and never calls setBaseRate. It applied
that factor whether or not the rule uses TotalRate, so a TotalRate rule with a
symmetric reaction center ran at a fraction of the rate the model asks for. For a
homodimer the fraction is one half.
TotalRate means the rate law gives the whole propensity of the rule. The symmetry
factor corrects a counting problem, namely that a symmetric pattern matches the
same reaction more than once. Under TotalRate there is no counting to correct,
because the rate is stated outright, so the factor should not be applied.
Measured on the model in issue #90. A homodimer states a total rate of 0.02 over
1000 time units starting from 400 free monomers, so about 20 reactions should
fire, each consuming two monomers, which leaves about 360.
The check has to sit where the propensity is computed. NFinput calls
setTotalRateFlag well after both the ReactionClass constructor and setBaseRate,
so the flag is still false at both of those points and a check placed there would
never fire.
Only FunctionalRxnClass needs the change. BioNetGen forces every TotalRate rate
law into a function even when the rate is a bare constant, so a TotalRate rule
never reaches the elementary path where setBaseRate applies the factor. BioNetGen
also rejects TotalRate on Sat, MM and Hill rate laws, on Arrhenius, and on local
functions. BasicRxnClass is left alone. It reads the folded baseRate in its own
TotalRate branch, but BioNetGen cannot produce that combination, and matching
v1.14.3 for hand written XML is more useful than an unreachable correction.
The new fixture in test/symmetry puts a symmetric and an asymmetric TotalRate
pool on the same expected survivor count, 2000 out of 4000. The asymmetric pool
has a symmetry factor of 1, so no change to the factor can move it, which
separates a working fix from a change that breaks TotalRate some other way.
validate.py gains a matching test next to the existing symmetry tests.
Checked both ways against a build of master. The new test fails at 2990.8 without
the change and passes at about 2000 with it. The three existing symmetry tests
pass either way.
This came in with "Apply the reaction center symmetry factor on every rate law,
not just Ele" from PR #89. That change was correct for ordinary rate laws. The
TotalRate case looks like it was missed because the same member is shared.