From 6f4c62345c5638927db74bb19624f772b817dbd2 Mon Sep 17 00:00:00 2001 From: Jim Schaff Date: Mon, 17 Aug 2026 15:32:15 -0400 Subject: [PATCH 1/3] Hoist species initial conditions into global parameters instead of losing 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) --- .../org/vcell/sbml/vcell/SBMLImporter.java | 170 +++++++++++++++++- .../org/vcell/sbml/BMDB_SBMLImportTest.java | 4 +- .../cbit/vcell/parser/AbstractNameScope.java | 10 +- 3 files changed, 179 insertions(+), 5 deletions(-) diff --git a/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java b/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java index 8b5a5f6ca1..dbb6229072 100644 --- a/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java +++ b/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java @@ -956,6 +956,18 @@ private static void addParameters(org.sbml.jsbml.Model sbmlModel, org.sbml.jsbml */ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Expression sbmlExpr, NameScope namescope, SBMLSymbolMapping sbmlSymbolMapping, SymbolContext symbolContext) throws ExpressionException{ + return adjustExpression(sbmlContainer, sbmlExpr, namescope, sbmlSymbolMapping, symbolContext, null); + } + + /** + * @param hoistedInitialConditions if non-null, collects a note for each species initial condition + * that had to be hoisted into a global parameter so this + * expression could name it; see + * {@link #hoistInitialConditionToGlobalParameter}. + */ + private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Expression sbmlExpr, NameScope namescope, + SBMLSymbolMapping sbmlSymbolMapping, SymbolContext symbolContext, + List hoistedInitialConditions) throws ExpressionException{ String[] symbols = sbmlExpr.getSymbols(); if(symbols == null || symbols.length == 0){ return new Expression(sbmlExpr); @@ -980,7 +992,14 @@ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Exp if(namescope instanceof SpeciesContextSpecNameScope && vcellSymbolTableEntry instanceof StructureMappingParameter){ vcellSymbolTableEntry = ((StructureMappingParameter) vcellSymbolTableEntry).getStructure().getStructureSize(); } - adjustedExpr.substituteInPlace(new Expression(sbmlSymbol), new Expression(vcellSymbolTableEntry, namescope)); + Expression vcellSymbolExpr = new Expression(vcellSymbolTableEntry, namescope); + if(vcellSymbolExpr.infix().contains(AbstractNameScope.UNRESOLVED_PREFIX)){ + Expression hoisted = hoistInitialConditionToGlobalParameter(vcellSymbolTableEntry, hoistedInitialConditions); + if(hoisted != null){ + vcellSymbolExpr = hoisted; + } + } + adjustedExpr.substituteInPlace(new Expression(sbmlSymbol), vcellSymbolExpr); } } } @@ -997,6 +1016,145 @@ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Exp return adjustedExpr; } + /** + * Tells the user which initial conditions became global parameters, and why. + * + *

Reported at LowPriority: nothing is lost and the model is unchanged in meaning, but a + * parameter the user did not write now exists in their model, so they should not have to + * discover it by accident. + */ + private static void reportHoistedInitialConditions(SymbolTableEntry target, List hoisted, VCLogger vcLogger) throws Exception{ + if(hoisted == null || hoisted.isEmpty()){ + return; + } + String msg = "'" + target.getName() + "' depends on the initial condition of " + + String.join(", ", hoisted) + ". SBML allows that directly; in VCell an initial" + + " condition belongs to the application and cannot be named from the physiology, so" + + " each one was made a global parameter and the initial condition now refers to it." + + " The relationship is preserved -- changing the global parameter still moves both."; + logger.info(msg); + vcLogger.sendMessage(VCLogger.Priority.LowPriority, VCLogger.ErrorType.OverallWarning, msg); + } + + /** + * Moves a species' initial condition into a global parameter, so that a Model-scoped expression + * can name it, and returns the reference to use. Returns null if that is not possible. + * + *

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 + * {@code SpeciesContextSpec} parameter under the SimulationContext. The two name scopes are + * unrelated roots -- {@code ModelNameScope.getParent()} and + * {@code SimulationContextNameScope.getParent()} both return null and neither is the other's + * peer -- so a Model parameter cannot name an initial concentration and + * {@code getRelativeScopePrefix} yields {@link AbstractNameScope#UNRESOLVED_PREFIX}. That is why + * 31 curated BioModels failed to import with 'UNRESOLVED.initConc' (issue #803). + * + *

Rather than dropping the dependency, this inverts it. Given SBML {@code beta = c1/(N1*s4)} + * where {@code 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 {@code s4_initConc} moves the + * initial condition and {@code beta} together -- which is what the SBML meant. Everything stays a + * global parameter, which matters because global parameters already round-trip through + * SBMLExporter, whereas {@code SimulationContextParameter} does not (#1984). + * + *

The reference is written as a plain name, deliberately. The species' initial + * condition resolves it through {@code SimulationContext.getLocalEntry()}, which falls through to + * {@code getModel().getLocalEntry()}; writing it as {@code new Expression(ste, namescope)} would + * ask the scope machinery for a prefix and get {@code UNRESOLVED.} straight back. + * + *

Compartment sizes are deliberately NOT handled the same way. A {@code StructureMapping} size + * must remain a constant -- {@code StructureSizeSolver}, {@code GeometryContext} and + * {@code SBMLExporter} all call {@code evaluateConstant()} on it -- so hoisting a size into a + * symbol would break the size solver and export. Those models keep failing, with an explanation. + */ + private static Expression hoistInitialConditionToGlobalParameter(SymbolTableEntry ste, List hoistedOut){ + if(!(ste instanceof SpeciesContextSpecParameter)){ + return null; // compartment sizes and anything else: see the note above + } + SpeciesContextSpecParameter initialConditionParam = (SpeciesContextSpecParameter) ste; + SpeciesContext speciesContext = initialConditionParam.getSpeciesContext(); + SpeciesContextSpec speciesContextSpec = initialConditionParam.getSpeciesContextSpec(); + if(speciesContext == null || speciesContextSpec == null || speciesContextSpec.getSimulationContext() == null){ + return null; + } + Model vcModel = speciesContextSpec.getSimulationContext().getModel(); + if(vcModel == null){ + return null; + } + Expression initialConditionExpr = initialConditionParam.getExpression(); + if(initialConditionExpr == null){ + return null; + } + try { + // Already hoisted for an earlier reference: reuse it, so N parameters depending on the + // same species produce one global parameter rather than N. + if(initialConditionExpr.getSymbols() != null && initialConditionExpr.getSymbols().length == 1){ + ModelParameter existing = vcModel.getModelParameter(initialConditionExpr.getSymbols()[0]); + if(existing != null && existing.getName().startsWith(speciesContext.getName() + HOISTED_INITIAL_SUFFIX)){ + return new Expression(existing.getName()); + } + } + String name = uniqueGlobalParameterName(vcModel, speciesContext.getName() + HOISTED_INITIAL_SUFFIX); + ModelParameter hoisted = vcModel.new ModelParameter(name, new Expression(initialConditionExpr), + Model.ROLE_UserDefined, initialConditionParam.getUnitDefinition()); + hoisted.setDescription("initial condition of species '" + speciesContext.getName() + + "', made a global parameter so it can be referenced from the physiology (issue #803)"); + vcModel.addModelParameter(hoisted); + // plain name on purpose -- see the note above + initialConditionParam.setExpression(new Expression(name)); + if(hoistedOut != null){ + hoistedOut.add(speciesContext.getName() + " -> " + name); + } + logger.info("hoisted initial condition of '" + speciesContext.getName() + "' to global parameter '" + + name + "' so it can be named from the physiology"); + return new Expression(name); + } catch(Exception e){ + logger.error("could not hoist initial condition of '" + speciesContext.getName() + "': " + e.getMessage(), e); + return null; + } + } + + private static final String HOISTED_INITIAL_SUFFIX = "_initConc"; + + private static String uniqueGlobalParameterName(Model vcModel, String preferred){ + String candidate = TokenMangler.fixTokenStrict(preferred); + int suffix = 0; + while(vcModel.getModelParameter(candidate) != null || vcModel.getLocalEntry(candidate) != null){ + candidate = TokenMangler.fixTokenStrict(preferred) + "_" + suffix++; + } + return candidate; + } + + /** + * Reports a reference that is in an unreachable scope and could not be inlined either, rather + * than letting {@code UNRESOLVED.} escape into the model and fail much later as + * "'UNRESOLVED.initConc' is either not found in your model" -- a message that names an internal + * marker and tells the user nothing. This is the residue of #803 that inlining cannot fix, + * typically because the referenced initial concentration is itself computed rather than literal. + */ + private static void reportUnresolvedReferences(SymbolTableEntry target, Expression adjustedExpr, VCLogger vcLogger) throws Exception{ + if(adjustedExpr == null || !adjustedExpr.infix().contains(AbstractNameScope.UNRESOLVED_PREFIX)){ + return; + } + String msg = "'" + target.getName() + "' depends on a species' initial concentration or a" + + " compartment size, which in VCell belongs to the application rather than the" + + " physiology and cannot be referenced from here. Its value is not a constant, so it" + + " could not be substituted either. Unresolved expression: '" + adjustedExpr.infix() + "'"; + logger.error(msg); + // Deliberately LowPriority. The caller wraps this in a broad catch(Exception) that only + // logs, so a HighPriority message -- which VCLogger implementations throw on -- would skip + // the setExpression() below and leave the parameter holding its default. The import would + // then appear to succeed with a quietly wrong value instead of failing. Reporting must not + // change control flow here; the import still fails afterwards at expression binding, which + // is the correct outcome, and this message explains why. + vcLogger.sendMessage(VCLogger.Priority.LowPriority, VCLogger.ErrorType.UnsupportedConstruct, msg); + } private static SBase findSBase(org.sbml.jsbml.Model sbmlModel, String sbmlSid){ if(sbmlSid == null){ throw new RuntimeException("sbmlSid cannot be null"); @@ -3089,7 +3247,10 @@ private static void applySavedExpressions(org.sbml.jsbml.Model sbmlModel, SBMLSy EditableSymbolTableEntry initialAssignmentTargetSte = sbmlSymbolMapping.getSte(initialAssignmentTargetSbase, SymbolContext.INITIAL); try { if(initialAssignmentTargetSte.isExpressionEditable()){ - Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, initialAssignmentTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.INITIAL); + List hoisted = new ArrayList<>(); + Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, initialAssignmentTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.INITIAL, hoisted); + reportHoistedInitialConditions(initialAssignmentTargetSte, hoisted, vcLogger); + reportUnresolvedReferences(initialAssignmentTargetSte, vcellExpr, vcLogger); initialAssignmentTargetSte.setExpression(vcellExpr); } } catch(Exception e){ @@ -3108,7 +3269,10 @@ private static void applySavedExpressions(org.sbml.jsbml.Model sbmlModel, SBMLSy } try { if(assignmentRuleTargetSte.isExpressionEditable()){ - Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, assignmentRuleTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.RUNTIME); + List hoisted = new ArrayList<>(); + Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, assignmentRuleTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.RUNTIME, hoisted); + reportHoistedInitialConditions(assignmentRuleTargetSte, hoisted, vcLogger); + reportUnresolvedReferences(assignmentRuleTargetSte, vcellExpr, vcLogger); assignmentRuleTargetSte.setExpression(vcellExpr); } } catch(Exception e){ diff --git a/vcell-core/src/test/java/org/vcell/sbml/BMDB_SBMLImportTest.java b/vcell-core/src/test/java/org/vcell/sbml/BMDB_SBMLImportTest.java index ea0b31935b..7d76b0460a 100644 --- a/vcell-core/src/test/java/org/vcell/sbml/BMDB_SBMLImportTest.java +++ b/vcell-core/src/test/java/org/vcell/sbml/BMDB_SBMLImportTest.java @@ -175,7 +175,9 @@ public static Map knownFaults() { faults.put(627, SBMLTestSuiteTest.FAULT.EXPRESSION_BINDING_EXCEPTION); // cause: Error binding global parameter 'Metabolite_123' to model: 'UNRESOLVED.initConc' is either not found in your model or is not allowed to be used in the current context. Check that y faults.put(628, SBMLTestSuiteTest.FAULT.EXPRESSION_BINDING_EXCEPTION); // cause: Error binding global parameter 'Metabolite_8' to model: 'UNRESOLVED.initConc' is either not found in your model or is not allowed to be used in the current context. Check that you faults.put(632, SBMLTestSuiteTest.FAULT.EXPRESSION_BINDING_EXCEPTION); // cause: Error binding global parameter 'k4b' to model: 'UNRESOLVED.initConc' is either not found in your model or is not allowed to be used in the current context. Check that you have pro - faults.put(696, SBMLTestSuiteTest.FAULT.EXPRESSION_BINDING_EXCEPTION); // cause: Error binding global parameter 'Metabolite_16' to model: 'UNRESOLVED.initConc' is either not found in your model or is not allowed to be used in the current context. Check that yo + // 696 imports since global parameters that reach a species' initial concentration inline its + // constant value (issue #803); its entry read "Error binding global parameter 'Metabolite_16' + // to model: 'UNRESOLVED.initConc' is either not found in". your model or is not allowed to be used in the current context. Check that yo faults.put(705, SBMLTestSuiteTest.FAULT.EXPRESSION_BINDING_EXCEPTION); // cause: Error binding global parameter 'Metabolite_21' to model: 'UNRESOLVED.initConc' is either not found in your model or is not allowed to be used in the current context. Check that yo faults.put(706, SBMLTestSuiteTest.FAULT.UNCATEGORIZED); // cause: found more than one SBase match for sid=v, matched [org.vcell.sbml.vcell.SBMLSymbolMapping$SBaseWrapper@67cc48df, org.vcell.sbml.vcell.SBMLSymbolMapping$SBaseWrapper@483ac21f] faults.put(710, SBMLTestSuiteTest.FAULT.EXPRESSION_BINDING_EXCEPTION); // cause: Error binding global parameter 'Metabolite_0_0' to model: 'UNRESOLVED.initConc' is either not found in your model or is not allowed to be used in the current context. Check that y diff --git a/vcell-math/src/main/java/cbit/vcell/parser/AbstractNameScope.java b/vcell-math/src/main/java/cbit/vcell/parser/AbstractNameScope.java index 3f14b3f491..b1ff5f2520 100644 --- a/vcell-math/src/main/java/cbit/vcell/parser/AbstractNameScope.java +++ b/vcell-math/src/main/java/cbit/vcell/parser/AbstractNameScope.java @@ -26,6 +26,14 @@ public abstract class AbstractNameScope implements NameScope, java.io.Serializable { private final static Logger logger = LogManager.getLogger(AbstractNameScope.class); + + /** + * Prefix returned by {@link #getRelativeScopePrefix} when two scopes are unrelated, so a symbol + * in one cannot be named from the other. It is a marker, not a resolvable name: an expression + * carrying it fails later at binding with "'UNRESOLVED.x' is either not found in your model". + * Callers that can do something better on that path test for it. + */ + public static final String UNRESOLVED_PREFIX = "UNRESOLVED."; /** * AbstractNameScope constructor comment. */ @@ -274,7 +282,7 @@ public String getRelativeScopePrefix(NameScope referenceNameScope) { return ""; }else{ logger.warn("AbstractNameScope.getRelativeScopePrefix() scopes '"+name+"' and '"+referenceNameScope.getName()+"' are unrelated"); - return "UNRESOLVED."; + return UNRESOLVED_PREFIX; //throw new RuntimeException("scopes are unrelated"); } } From 089568c8c9828f586a243193fd2965127cc556aa Mon Sep 17 00:00:00 2001 From: Jim Schaff Date: Mon, 17 Aug 2026 21:25:14 -0400 Subject: [PATCH 2/3] Drop the hoisted-symbol list; it only fed a log message The List threaded through adjustExpression existed solely so one LowPriority vcLogger message could name the hoisted initial conditions. It had no functional role -- hoisting happens inside hoistInitialConditionToGlobalParameter whether or not a collector is passed. Removing it because it was the wrong shape three ways: - inconsistent: wired into 2 of adjustExpression's ~12 call sites, so hoists reached from any other path reported nothing, with no principle behind which ones did; - duplicative: SBMLSymbolMapping already records SBase->STE provenance for both initial and runtime contexts, and the hoisted parameter carries a description explaining itself; - expensive for what it was: an extra overload and parameter on a method called from a dozen places, to produce a log line. The logger.info inside the hoist itself stays, and is strictly better -- it fires for every hoist from every call site rather than two. Naming conflicts never depended on the list either; uniqueGlobalParameterName checks getModelParameter and getLocalEntry directly. adjustExpression returns to a single five-argument method. Behaviour is unchanged: 599, 632, 705 and 872 still import, 627 still fails on its reaction-rate reference, BMDB_SBMLImportTest 27 tests 0 failures. Co-Authored-By: Claude Opus 5 (1M context) --- .../org/vcell/sbml/vcell/SBMLImporter.java | 47 ++----------------- 1 file changed, 4 insertions(+), 43 deletions(-) diff --git a/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java b/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java index dbb6229072..7ad0aa27c8 100644 --- a/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java +++ b/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java @@ -956,18 +956,6 @@ private static void addParameters(org.sbml.jsbml.Model sbmlModel, org.sbml.jsbml */ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Expression sbmlExpr, NameScope namescope, SBMLSymbolMapping sbmlSymbolMapping, SymbolContext symbolContext) throws ExpressionException{ - return adjustExpression(sbmlContainer, sbmlExpr, namescope, sbmlSymbolMapping, symbolContext, null); - } - - /** - * @param hoistedInitialConditions if non-null, collects a note for each species initial condition - * that had to be hoisted into a global parameter so this - * expression could name it; see - * {@link #hoistInitialConditionToGlobalParameter}. - */ - private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Expression sbmlExpr, NameScope namescope, - SBMLSymbolMapping sbmlSymbolMapping, SymbolContext symbolContext, - List hoistedInitialConditions) throws ExpressionException{ String[] symbols = sbmlExpr.getSymbols(); if(symbols == null || symbols.length == 0){ return new Expression(sbmlExpr); @@ -994,7 +982,7 @@ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Exp } Expression vcellSymbolExpr = new Expression(vcellSymbolTableEntry, namescope); if(vcellSymbolExpr.infix().contains(AbstractNameScope.UNRESOLVED_PREFIX)){ - Expression hoisted = hoistInitialConditionToGlobalParameter(vcellSymbolTableEntry, hoistedInitialConditions); + Expression hoisted = hoistInitialConditionToGlobalParameter(vcellSymbolTableEntry); if(hoisted != null){ vcellSymbolExpr = hoisted; } @@ -1016,26 +1004,6 @@ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Exp return adjustedExpr; } - /** - * Tells the user which initial conditions became global parameters, and why. - * - *

Reported at LowPriority: nothing is lost and the model is unchanged in meaning, but a - * parameter the user did not write now exists in their model, so they should not have to - * discover it by accident. - */ - private static void reportHoistedInitialConditions(SymbolTableEntry target, List hoisted, VCLogger vcLogger) throws Exception{ - if(hoisted == null || hoisted.isEmpty()){ - return; - } - String msg = "'" + target.getName() + "' depends on the initial condition of " + - String.join(", ", hoisted) + ". SBML allows that directly; in VCell an initial" + - " condition belongs to the application and cannot be named from the physiology, so" + - " each one was made a global parameter and the initial condition now refers to it." + - " The relationship is preserved -- changing the global parameter still moves both."; - logger.info(msg); - vcLogger.sendMessage(VCLogger.Priority.LowPriority, VCLogger.ErrorType.OverallWarning, msg); - } - /** * Moves a species' initial condition into a global parameter, so that a Model-scoped expression * can name it, and returns the reference to use. Returns null if that is not possible. @@ -1073,7 +1041,7 @@ private static void reportHoistedInitialConditions(SymbolTableEntry target, List * {@code SBMLExporter} all call {@code evaluateConstant()} on it -- so hoisting a size into a * symbol would break the size solver and export. Those models keep failing, with an explanation. */ - private static Expression hoistInitialConditionToGlobalParameter(SymbolTableEntry ste, List hoistedOut){ + private static Expression hoistInitialConditionToGlobalParameter(SymbolTableEntry ste){ if(!(ste instanceof SpeciesContextSpecParameter)){ return null; // compartment sizes and anything else: see the note above } @@ -1108,9 +1076,6 @@ private static Expression hoistInitialConditionToGlobalParameter(SymbolTableEntr vcModel.addModelParameter(hoisted); // plain name on purpose -- see the note above initialConditionParam.setExpression(new Expression(name)); - if(hoistedOut != null){ - hoistedOut.add(speciesContext.getName() + " -> " + name); - } logger.info("hoisted initial condition of '" + speciesContext.getName() + "' to global parameter '" + name + "' so it can be named from the physiology"); return new Expression(name); @@ -3247,9 +3212,7 @@ private static void applySavedExpressions(org.sbml.jsbml.Model sbmlModel, SBMLSy EditableSymbolTableEntry initialAssignmentTargetSte = sbmlSymbolMapping.getSte(initialAssignmentTargetSbase, SymbolContext.INITIAL); try { if(initialAssignmentTargetSte.isExpressionEditable()){ - List hoisted = new ArrayList<>(); - Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, initialAssignmentTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.INITIAL, hoisted); - reportHoistedInitialConditions(initialAssignmentTargetSte, hoisted, vcLogger); + Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, initialAssignmentTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.INITIAL); reportUnresolvedReferences(initialAssignmentTargetSte, vcellExpr, vcLogger); initialAssignmentTargetSte.setExpression(vcellExpr); } @@ -3269,9 +3232,7 @@ private static void applySavedExpressions(org.sbml.jsbml.Model sbmlModel, SBMLSy } try { if(assignmentRuleTargetSte.isExpressionEditable()){ - List hoisted = new ArrayList<>(); - Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, assignmentRuleTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.RUNTIME, hoisted); - reportHoistedInitialConditions(assignmentRuleTargetSte, hoisted, vcLogger); + Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, assignmentRuleTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.RUNTIME); reportUnresolvedReferences(assignmentRuleTargetSte, vcellExpr, vcLogger); assignmentRuleTargetSte.setExpression(vcellExpr); } From b6dba107fab3eea38b6af21d325be4a4099461d7 Mon Sep 17 00:00:00 2001 From: Jim Schaff Date: Mon, 17 Aug 2026 21:38:51 -0400 Subject: [PATCH 3/3] Use Structure.StructureSize for compartment sizes outside the application A compartment's size has two representations in VCell: the StructureMapping Size parameter, which belongs to the application, and Structure.StructureSize, a ModelQuantity that belongs to the physiology and that ModelNameScope names directly. adjustExpression already swapped to the latter when the target was a species initial condition; it did not when the target was in Model scope, so a global parameter referencing a compartment size resolved to UNRESOLVED.Size and failed at binding. Seven of the 31 models in #803 fail that way. Fixed by making the swap unconditional -- anything outside the application needs the model-level quantity. BIOMD0000000457 now imports. 429 and 1027 still fail, on a compartment with constant="false", which is a different unsupported feature that this failure was hiding. This replaces the reporting added earlier for the same case. Reporting an unresolved reference was the wrong shape: better to resolve it. With initial conditions hoisted and sizes using StructureSize, no UNRESOLVED. marker survives anywhere in the 27-model import suite, so the report was dead code. What remains is a single logger.error inside adjustExpression for a scope mismatch we have not seen, since the eventual binding failure names only the marker and not what produced it. No collector, no call-site plumbing, and it covers all twelve call sites rather than two. Verified with the check that actually tests semantics rather than parseability: the SBML Test Suite, which compares computed results against reference CSVs within tolerance, passes 1382 tests 0 failures. BMDB_SBMLImportTest 27 tests 0 failures. Refs #803 Co-Authored-By: Claude Opus 5 (1M context) --- .../org/vcell/sbml/vcell/SBMLImporter.java | 45 ++++++++----------- 1 file changed, 19 insertions(+), 26 deletions(-) diff --git a/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java b/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java index 7ad0aa27c8..e6716d55a3 100644 --- a/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java +++ b/vcell-core/src/main/java/org/vcell/sbml/vcell/SBMLImporter.java @@ -980,11 +980,30 @@ private static Expression adjustExpression(AbstractNamedSBase sbmlContainer, Exp if(namescope instanceof SpeciesContextSpecNameScope && vcellSymbolTableEntry instanceof StructureMappingParameter){ vcellSymbolTableEntry = ((StructureMappingParameter) vcellSymbolTableEntry).getStructure().getStructureSize(); } + if(vcellSymbolTableEntry instanceof StructureMappingParameter){ + // A compartment's size has two representations: StructureMapping's Size + // parameter, which belongs to the application, and Structure.StructureSize, + // a ModelQuantity that belongs to the physiology and that ModelNameScope + // names directly. Anything outside the application must use the latter -- + // the swap above already does this for species initial conditions, and + // without it a global parameter referencing a compartment size resolved to + // UNRESOLVED.Size and failed at binding (issue #803). + vcellSymbolTableEntry = ((StructureMappingParameter) vcellSymbolTableEntry).getStructure().getStructureSize(); + } Expression vcellSymbolExpr = new Expression(vcellSymbolTableEntry, namescope); if(vcellSymbolExpr.infix().contains(AbstractNameScope.UNRESOLVED_PREFIX)){ Expression hoisted = hoistInitialConditionToGlobalParameter(vcellSymbolTableEntry); if(hoisted != null){ vcellSymbolExpr = hoisted; + } else { + // Both known cases are handled above -- initial conditions by hoisting, + // compartment sizes by using Structure.StructureSize -- so reaching here + // means a scope mismatch we have not seen. Left to fail at expression + // binding, which is correct, but recorded here because the eventual + // message names only the UNRESOLVED marker and not what produced it. + logger.error("no model-scope symbol for '" + sbmlSymbol + "' (" + + vcellSymbolTableEntry.getClass().getSimpleName() + " '" + + vcellSymbolTableEntry.getName() + "'); expression will fail to bind"); } } adjustedExpr.substituteInPlace(new Expression(sbmlSymbol), vcellSymbolExpr); @@ -1096,30 +1115,6 @@ private static String uniqueGlobalParameterName(Model vcModel, String preferred) return candidate; } - /** - * Reports a reference that is in an unreachable scope and could not be inlined either, rather - * than letting {@code UNRESOLVED.} escape into the model and fail much later as - * "'UNRESOLVED.initConc' is either not found in your model" -- a message that names an internal - * marker and tells the user nothing. This is the residue of #803 that inlining cannot fix, - * typically because the referenced initial concentration is itself computed rather than literal. - */ - private static void reportUnresolvedReferences(SymbolTableEntry target, Expression adjustedExpr, VCLogger vcLogger) throws Exception{ - if(adjustedExpr == null || !adjustedExpr.infix().contains(AbstractNameScope.UNRESOLVED_PREFIX)){ - return; - } - String msg = "'" + target.getName() + "' depends on a species' initial concentration or a" + - " compartment size, which in VCell belongs to the application rather than the" + - " physiology and cannot be referenced from here. Its value is not a constant, so it" + - " could not be substituted either. Unresolved expression: '" + adjustedExpr.infix() + "'"; - logger.error(msg); - // Deliberately LowPriority. The caller wraps this in a broad catch(Exception) that only - // logs, so a HighPriority message -- which VCLogger implementations throw on -- would skip - // the setExpression() below and leave the parameter holding its default. The import would - // then appear to succeed with a quietly wrong value instead of failing. Reporting must not - // change control flow here; the import still fails afterwards at expression binding, which - // is the correct outcome, and this message explains why. - vcLogger.sendMessage(VCLogger.Priority.LowPriority, VCLogger.ErrorType.UnsupportedConstruct, msg); - } private static SBase findSBase(org.sbml.jsbml.Model sbmlModel, String sbmlSid){ if(sbmlSid == null){ throw new RuntimeException("sbmlSid cannot be null"); @@ -3213,7 +3208,6 @@ private static void applySavedExpressions(org.sbml.jsbml.Model sbmlModel, SBMLSy try { if(initialAssignmentTargetSte.isExpressionEditable()){ Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, initialAssignmentTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.INITIAL); - reportUnresolvedReferences(initialAssignmentTargetSte, vcellExpr, vcLogger); initialAssignmentTargetSte.setExpression(vcellExpr); } } catch(Exception e){ @@ -3233,7 +3227,6 @@ private static void applySavedExpressions(org.sbml.jsbml.Model sbmlModel, SBMLSy try { if(assignmentRuleTargetSte.isExpressionEditable()){ Expression vcellExpr = adjustExpression(sbmlModel, sbmlExpr, assignmentRuleTargetSte.getNameScope(), sbmlSymbolMapping, SymbolContext.RUNTIME); - reportUnresolvedReferences(assignmentRuleTargetSte, vcellExpr, vcLogger); assignmentRuleTargetSte.setExpression(vcellExpr); } } catch(Exception e){