fix(codegen): a zero-arg observable call emits uncompilable C, and differentiates to zero - #71
Merged
Merged
Conversation
…fferentiates to zero BNGL accepts an Observable written as a zero-arg call (`divide()`) anywhere the bareword is valid, and BNG2.pl preserves whichever form the user wrote when it emits the .net. `ExprTkEvaluator::compile` strips `name()` -> `name` for every name registered as a scalar variable (`strip_empty_parens`, src/expression.cpp), so the interpreted engine has run these models since issue #28. The two paths that consume the expression *strings* never got the same treatment. The codegen identifier tables set `eats_empty_parens` only for functions, so an observable was rewritten to its C scalar with the empty call still attached: double func__rateLaw1 = (1.0-obs_divide())*1.0; func[0] = (1.0-obs[1]())*1.0; /* _rateLaw1 */ error: called object type 'double' is not a function or function pointer Forward sensitivity needs the compiled RHS, so a failed build is a hard RuntimeError rather than a fallback: `ode/proliferation.bngl` was the last of the 585 `ode_fullnet` models whose sensitivity Simulator refused to construct -- the refusal PR #69 recorded as a separate defect. Every model name resolves to a scalar in the emitted C (`obs[j]`, `p[k]`, `y[i]`, `func[m]`, `current_derivs[i]`, `M_PI`), so all of them now eat the parens; `eats_empty_parens=False` is left only for the built-ins that really are C functions or operators (fabs/log/round/fmax/ fmin, &&/||/!), where an empty argument list is not valid ExprTk to begin with. The same missing strip reached the sympy-facing differentiator, and there it was worse than a build failure. `parse_expr` reads `divide()` as an applied undefined function that shares no symbol with the bareword, so d/d(divide) of `100*divide()` came back empty: a silently zero analytical-Jacobian entry when such a body is a rate law, and a silently zero d func/dtheta in the #198 expression output sensitivities -- both reported as ordinary results, neither refused. A body like `scale*Atot()` instead differentiated to something the C printer cannot render, i.e. a spurious "not representable" decline. `_preprocess_exprtk` now strips for the sympy path too, after the `time()` rewrite that needs the parens. Fixing only the emitters would have converted a loud failure into a wrong number. Corpus: the sensitivity Simulator goes from 584/585 to 585/585, and proliferation's compiled trajectory matches the interpreted RHS to 4.5e-11 over t in [0, 20]. It is also the only corpus model that calls an observable or a parameter as `name()` inside a function body -- inter-function `f()` references arrive already resolved to the bareword by the engine, which is why the hole survived issue #28. Tests: a new fixture distilled from proliferation, elementary so the analytic sens RHS engages and the call-form bodies are pure outputs. It pins the emitted C against a call on any C scalar, and checks d func/dtheta against the model's closed forms for both the constant-coefficient shape (which used to come back as exactly 0.0) and the parameter-coefficient shape (which used to decline), plus unit coverage of both lookup tables and guards that abs/ln/max and `time()` keep their parens. Stashing the two source files fails 9 of the 11 new tests; the two guards pass either way.
wshlavacek
force-pushed
the
claude/gracious-curie-88b62b
branch
from
July 26, 2026 22:48
b985cc8 to
d5520e8
Compare
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.
The defect
proliferation.bnglhas an observable nameddivide(group 2) that its functionsreference with BNGL's zero-arg call syntax:
BNGL accepts an Observable written as
obs()anywhere the bareword is valid(
bng2/Perl2/Expression.pm:870-927) and BNG2.pl preserves whichever form the userwrote.
ExprTkEvaluator::compilestripsname()→namefor every nameregistered as a scalar variable (
strip_empty_parens,src/expression.cpp), sothe interpreted engine has run these models since issue #28. The two paths that
consume the expression strings never got the same treatment.
This is the refusal #69 recorded as "an unrelated defect (zero-arg observable call
emitted as
obs[1]())" — the last of the 585ode_fullnetmodels whosesensitivity Simulator would not construct.
Fix 1 — the emitters (the reported symptom)
_build_ident_lookupand_build_ident_lookup_modelseteats_empty_parensonlyfor functions, so an observable was rewritten to its C scalar with the call left
attached. Every model name resolves to a scalar in the emitted C (
obs[j],p[k],y[i],func[m],current_derivs[i],M_PI), so all of them now eat the parens.Falsesurvives only for the built-ins that really are C functions or operators(
fabs/log/round/fmax/fmin,&&/||/!), where an empty argument listis not valid ExprTk to begin with.
Fix 2 — the differentiator (found while fixing 1, and worse)
The same missing strip reached
_jacobian._preprocess_exprtk. sympy'sparse_exprreadsdivide()as an applied undefined function that shares nosymbol with the bareword, so
∂/∂dividecame back empty:differentiate_rate_law("100*divide()"){}— silently zero Jacobian entry{'divide': 100}differentiate_rate_law("100*divide"){'divide': 100}{'divide': 100}∂/∂θof100*Atot()(#198){}— silently zerod func/dθ, no error{'observable': {'Atot': '100.0'}}∂/∂θofscale*Atot()(#198){'observable': …, 'param': …}Both silent shapes were reported as ordinary results. Fixing only the emitters
would have converted a loud build failure into a wrong number, so this half is
part of the same change. The strip runs after the
time()rewrite, which needsthe parens it matches on.
Validation
benchmarks/suites/ode_fullnet/nets(all 585 constructed, 0 failures).proliferation's compiled trajectory matches the interpreted RHSto 4.5e-11 over
t ∈ [0, 20]atrtol=1e-10.functionsblock against itsgroups/parametersblocks,proliferationis the only model that calls anobservable or parameter as
name()inside a function body. Inter-functionf()references (73 models) arrive already resolved to the bareword by the engine, so
they are unaffected — which is why this hole survived issue Make method="integration" the default for steady_state(): the two-tier KINSOL solve costs 1.4-3.9x more on every benchmarked model #28.
Tests
New fixture
tests/data/obs_zero_arg_call_sens.net, distilled fromproliferationand elementary so the analytic sens RHS engages and the call-formbodies are pure outputs, with closed forms in the header. 11 tests added to
test_obs_zero_arg_call.py:(named locals from the
.netRHS, arrays from the model-based emitters);d func/dθmatches the closed forms for the constant-coefficient shape (used tobe exactly
0.0) and the parameter-coefficient shape (used to decline);_pi()/ rateOf cases;abs/ln/max/tanhandtime()keep their parens.git stash push python/bngsim/_codegen.py python/bngsim/_jacobian.pyfails 9 ofthe 11; the two guards pass either way.