Skip to content

Say what is wrong when SBML names a reaction in an expression - #1983

Merged
jcschaff merged 1 commit into
masterfrom
fix/sbml-reaction-rate-reference
Aug 17, 2026
Merged

Say what is wrong when SBML names a reaction in an expression#1983
jcschaff merged 1 commit into
masterfrom
fix/sbml-reaction-rate-reference

Conversation

@jcschaff

Copy link
Copy Markdown
Member

Addresses the 961 half of #1981. 872 is a different bug — see below.

What was wrong

SBML lets a <ci> name a reaction, and doing so denotes that reaction's rate (L3V2 §3.4.3). VCell has no symbol for a reaction rate, so the reference survived translation as an ordinary name and nothing noticed until the finished BioModel bound its expressions:

Error binding global parameter 'rateOf_re15' to model: 're15' is either not found in your model or is not allowed to be used in the current context.

That names the reaction as if it were a typo, says nothing about rates, and points at the parameter rather than at the construct that is unsupported. The limitation is real; only the report was bad. Now:

Reaction rate reference: reaction 're37' is named in an expression, which in SBML denotes that reaction's rate. Reaction rates cannot be referenced in expressions in VCell at this time. Expression: 're37'

Correcting #1981: these are not the same bug

I claimed in #1981 that 872 and 961 looked like one importer bug. Having read both models, that was wrong:

model expression references mechanism
961 reaction re15 SBML reaction-rate reference — fixed here
872 species s4 in a global parameter's initialAssignment maps to the species' initial concentration, which lives in the application (SimulationContext) scope; a Model-level parameter cannot see it, so AbstractNameScope.getRelativeScopePrefix() yields the UNRESOLVED. marker

872 is a name-scope problem, not a symbol-resolution one, and it is shared by a large number of models — the fault table already lists many UNRESOLVED.initConc / UNRESOLVED.Size entries. It needs its own change and is deliberately not in this PR.

Also: it is not the rateOf csymbol

Worth recording, since #1981's title suggested detecting rateOf. BIOMD0000000961 is a COPASI export whose parameters are merely named rateOf_re15; the actual construct is <ci>re15</ci> and the file contains no rateOf csymbol at all. Matching on the csymbol would never have fired.

Where the check lives

In getExpressionFromFormula, reached via ASTNode.getParentSBMLObject().getModel(). One check covers all thirteen call sites — kinetic laws, assignment/rate rules, initial assignments, events, stoichiometry math — with no signature changes.

It skips names that a containing kinetic law declares as local parameters: those are a separate SBML namespace and may shadow a reaction sid without referencing a rate. That was a false-positive path I could see in my own first draft.

The verdict leads and the expression trails deliberately — consumers truncate this message (BMDB_SBMLImportTest keeps 180 chars, and model 731's expression alone is ~150), so a message ending in the diagnosis got categorised UNCATEGORIZED. It also reads better in a log.

Three pre-existing test bugs, fixed because the regression could not be trusted without them

  1. BMDB_SBMLImportTest.testCases() was pinned to one model. A leftover debug filter, && n==264, has restricted the whole 109-entry fault suite to a single model since 2025-04-09 (4e05dbff34) — about 16 months. 27 models run again. The committed model set is 29, so this pulls in no new downloads and no new network dependency.
  2. Model 739's fault entry was dead code — it sat on the same physical line as model 731's trailing // comment, so it was commented out and never registered. Split onto its own line.
  3. Model 596 removed from the fault table — it now imports cleanly. Its recorded cause was the nested-annotation ClassCastException fixed in jsbml 1.6.1-VCELL-4 (fix JSBML library processing errors for Annotations #1461), so the entry was stale as of that release.

Verification

run result
BMDB_SBMLImportTest, filter restored, with this change 27 tests, 0 failures
same suite without the importer change (control) 1 failure — model 596 only
vcell-core Fast group, CI parallel flags 547 tests, 1 error (VCellDataTest poetry noise, environmental)

The control is the important one: no model went from passing to failing. 731 failed before and fails now — it just says why.

🤖 Generated with Claude Code

SBML lets a <ci> name a reaction, and doing so denotes that reaction's rate (L3V2 3.4.3).
VCell has no symbol for a reaction rate, so such a reference survived translation as an
ordinary name and nothing noticed until the finished BioModel bound its expressions:

    Error binding global parameter 'rateOf_re15' to model: 're15' is either not found in
    your model or is not allowed to be used in the current context.

That names the reaction as if it were a typo, says nothing about rates, and points at the
parameter rather than the construct that is unsupported. The limitation is real; only the
report was bad. Now, from the importer, where the SBML is still in hand:

    Reaction rate reference: reaction 're37' is named in an expression, which in SBML denotes
    that reaction's rate. Reaction rates cannot be referenced in expressions in VCell at this
    time. Expression: 're37'

Reported at HighPriority, like the other unsupported constructs -- the alternative is a model
that silently omits the dependency.

The verdict deliberately leads and the expression trails, because consumers truncate this
message. BMDB_SBMLImportTest keeps 180 characters, and model 731's expression alone is about
150, so a message ending in the diagnosis was categorised UNCATEGORIZED. That is also better
for a human reading a log.

Placed in getExpressionFromFormula, reached via ASTNode.getParentSBMLObject().getModel(), so
one check covers all thirteen call sites -- kinetic laws, rules, initial assignments, events,
stoichiometry math -- with no signature changes. Skips names that a containing kinetic law
declares as local parameters, since those are a separate namespace and may shadow a reaction
sid without referencing a rate.

Note this is the plain-<ci> form, NOT the rateOf csymbol. BIOMD0000000961, which prompted
this, is a COPASI export whose parameters are merely NAMED rateOf_re15 while the construct is
<ci>re15</ci>; it contains no rateOf csymbol at all. Matching the csymbol would not have
caught it.

Two models change category, neither from passing to failing:

  961  was EXPRESSION_BINDING_EXCEPTION on 're15', now names the rate reference
  731  same, on 'func_TRegs_Production_from_CD4' -- it had the same bug all along

Also in this commit, because the regression could not be trusted without them:

  - BMDB_SBMLImportTest.testCases() carried a leftover debug filter, "&& n==264", pinning the
    whole suite to a single model since 2025-04-09 (4e05dbf). 27 models run again. The
    committed model set is 29, so this does not pull in new downloads.
  - Model 739's fault entry sat on the same physical line as 731's trailing // comment, so it
    was commented out and never registered. Split onto its own line.
  - Model 596 is removed from the fault table: it now imports cleanly, its recorded cause
    being the nested-annotation ClassCastException fixed in jsbml 1.6.1-VCELL-4 (issue #1461).

Verified: BMDB_SBMLImportTest 27 tests 0 failures with the filter restored; without this
change the same suite fails only on 596, confirming no model regressed. vcell-core Fast group
547 tests, 1 error (VCellDataTest poetry noise, environmental).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jcschaff
jcschaff merged commit c85875c into master Aug 17, 2026
8 checks passed
@jcschaff
jcschaff deleted the fix/sbml-reaction-rate-reference branch August 17, 2026 13:06
jcschaff added a commit that referenced this pull request Aug 17, 2026
…sing them

SBML has one flat namespace; VCell separates physiology from application. An SBML global
parameter becomes a Model parameter, while a species' initial concentration is a
SpeciesContextSpec parameter under the SimulationContext. Those two name scopes are unrelated
roots -- ModelNameScope.getParent() and SimulationContextNameScope.getParent() both return
null, and neither is the other's peer -- so a Model parameter cannot name an initial
concentration, getRelativeScopePrefix yields the UNRESOLVED. marker, and the import dies much
later with

    Error binding global parameter 'beta' to model: 'UNRESOLVED.initConc' is either not found
    in your model or is not allowed to be used in the current context.

31 curated BioModels fail this way (issue #803, open since 2023).

Rather than dropping the dependency or freezing it as a number, invert it. For SBML
beta = c1/(N1*s4) where s4 is a species:

    before   s4.initConc = 250000              beta = c1/(N1 * UNRESOLVED.initConc)  [broken]
    after    s4_initConc = 250000   (global)   s4.initConc = s4_initConc
                                               beta = c1/(N1 * s4_initConc)          [exact]

Nothing is lost. The relationship stays symbolic, so scanning s4_initConc moves the initial
condition and beta together, which is what the SBML meant. Everything stays a global
parameter, which matters: global parameters already round-trip through SBMLExporter, whereas
SimulationContextParameter does not (#1984). Hoisting once per species, so N dependents
produce one parameter, not N.

The reference is written as a PLAIN NAME on purpose. A species' initial condition resolves it
through SimulationContext.getLocalEntry(), which falls through to getModel().getLocalEntry();
writing it as new Expression(ste, namescope) would ask the scope machinery for a prefix and
get UNRESOLVED. straight back. Verified both directions bind before building on it.

An earlier attempt inlined the constant value instead. Rejected: it freezes the dependency, so
the imported model reads as a magic number and an export no longer reproduces the source. It
was also strictly weaker -- it could only act when the initial condition was a literal, so
model 632, whose species initial condition is itself computed, stayed broken. Hoisting handles
it because it moves the expression, not the value.

Compartment sizes are deliberately NOT hoisted. A StructureMapping size must remain constant:
StructureSizeSolver (775, 786, 789), GeometryContext (419) and SBMLExporter (373, 387) all
call evaluateConstant() on it, so a symbol there would break the size solver and export. Those
models still fail, now with an explanation rather than a leaked UNRESOLVED marker.

Verified against the models: 599, 632, 705, 872 import (632 is the one inlining could not
do); 627 still fails, on a reaction-rate reference (#1983) that was hidden behind this one.
BMDB_SBMLImportTest 27 tests 0 failures, with 696 removed from the fault table because it now
passes. vcell-core Fast 547 tests, 1 error (VCellDataTest poetry noise, environmental).

AbstractNameScope gains a named constant for the "UNRESOLVED." literal so callers that can do
better on that path can test for it.

Refs #803, #1984

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant