diff --git a/scripts/vendor_exprtk.py b/scripts/vendor_exprtk.py index 0c94f2d..1ca516b 100755 --- a/scripts/vendor_exprtk.py +++ b/scripts/vendor_exprtk.py @@ -35,7 +35,6 @@ from pathlib import Path from typing import Any - RM_ROOT = Path(__file__).resolve().parents[1] VENDOR_DIR = RM_ROOT / "third_party" / "bngsim_expr" VENDOR_NOTE = VENDOR_DIR / "VENDOR" @@ -55,6 +54,11 @@ "source_path": "include/bngsim/expression.hpp", "description": "BNGsim ExprTkEvaluator public wrapper header", }, + "expr_compat_header": { + "path": "third_party/bngsim_expr/include/bngsim/expr_compat.hpp", + "source_path": "include/bngsim/expr_compat.hpp", + "description": "BNGsim BNG<->ExprTk compatibility primitives (GH #49)", + }, "expression_source": { "path": "third_party/bngsim_expr/src/expression.cpp", "source_path": "src/expression.cpp", @@ -67,8 +71,7 @@ def run(cmd: list[str], *, text: bool = True) -> subprocess.CompletedProcess: return subprocess.run( cmd, check=True, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, + capture_output=True, text=text, ) @@ -224,10 +227,7 @@ def build_metadata( def source_bytes_at_commit(bngsim: Path, commit: str) -> dict[str, bytes]: - return { - key: git_show(bngsim, commit, spec["source_path"]) - for key, spec in FILES.items() - } + return {key: git_show(bngsim, commit, spec["source_path"]) for key, spec in FILES.items()} def write_vendor_note(metadata: dict[str, Any]) -> None: @@ -311,8 +311,7 @@ def local_metadata_drift(metadata: dict[str, Any] | None) -> list[str]: continue if source_path != spec["source_path"]: drift.append( - f"files.{key}.source_path: expected {spec['source_path']!r}, " - f"found {source_path!r}" + f"files.{key}.source_path: expected {spec['source_path']!r}, found {source_path!r}" ) path = RM_ROOT / spec["path"] @@ -324,13 +323,11 @@ def local_metadata_drift(metadata: dict[str, Any] | None) -> list[str]: actual_bytes = len(data) if record.get("sha256") != actual_sha: drift.append( - f"files.{key}.sha256: expected {record.get('sha256')!r}, " - f"found {actual_sha!r}" + f"files.{key}.sha256: expected {record.get('sha256')!r}, found {actual_sha!r}" ) if record.get("bytes") != actual_bytes: drift.append( - f"files.{key}.bytes: expected {record.get('bytes')!r}, " - f"found {actual_bytes!r}" + f"files.{key}.bytes: expected {record.get('bytes')!r}, found {actual_bytes!r}" ) if not metadata_commit(metadata): @@ -344,9 +341,7 @@ def source_drift(bngsim: Path, commit: str) -> list[str]: source_data = git_show(bngsim, commit, spec["source_path"]) local_data = (RM_ROOT / spec["path"]).read_bytes() if source_data != local_data: - drift.append( - f"{spec['path']} differs from BNGsim {commit}:./{spec['source_path']}" - ) + drift.append(f"{spec['path']} differs from BNGsim {commit}:./{spec['source_path']}") return drift @@ -446,7 +441,7 @@ def main() -> int: metadata = build_metadata(bngsim, checkout_info, ref, commit, source_bytes) write_refresh(source_bytes, metadata) print( - "vendor_exprtk: refreshed 3 files plus VENDOR.json/VENDOR " + f"vendor_exprtk: refreshed {len(FILES)} files plus VENDOR.json/VENDOR " f"from BNGsim {ref} ({commit})" ) return 0 diff --git a/third_party/bngsim_expr/VENDOR b/third_party/bngsim_expr/VENDOR index 30286bb..500a865 100644 --- a/third_party/bngsim_expr/VENDOR +++ b/third_party/bngsim_expr/VENDOR @@ -9,12 +9,13 @@ target instead. Source repo : git@github.com:wshlavacek/PyBNF-Private.git Source subdir: bngsim -Pinned commit: 330cfcc4913a98bd45f2dc80da047621a2ed8092 +Pinned commit: 1a1d49da13f8cd723be213774c2a81f0da538c84 Metadata : third_party/bngsim_expr/VENDOR.json Vendored files (copied verbatim, no edits): third_party/exprtk/exprtk.hpp <- bngsim third_party/exprtk/exprtk.hpp third_party/bngsim_expr/include/bngsim/expression.hpp <- bngsim include/bngsim/expression.hpp + third_party/bngsim_expr/include/bngsim/expr_compat.hpp <- bngsim include/bngsim/expr_compat.hpp third_party/bngsim_expr/src/expression.cpp <- bngsim src/expression.cpp Refresh / drift-check diff --git a/third_party/bngsim_expr/VENDOR.json b/third_party/bngsim_expr/VENDOR.json index 8ed7ee7..2fb6610 100644 --- a/third_party/bngsim_expr/VENDOR.json +++ b/third_party/bngsim_expr/VENDOR.json @@ -5,11 +5,11 @@ "name": "BNGsim", "repo_remote": "git@github.com:wshlavacek/PyBNF-Private.git", "repo_subdir": "bngsim", - "branch_or_ref": "330cfcc4913a98bd45f2dc80da047621a2ed8092", + "branch_or_ref": "1a1d49da13f8cd723be213774c2a81f0da538c84", "current_branch": "feature/bngsim", - "commit": "330cfcc4913a98bd45f2dc80da047621a2ed8092" + "commit": "1a1d49da13f8cd723be213774c2a81f0da538c84" }, - "imported_at_utc": "2026-05-23T19:17:39+00:00", + "imported_at_utc": "2026-06-20T02:10:39+00:00", "files": { "exprtk_header": { "path": "third_party/exprtk/exprtk.hpp", @@ -25,11 +25,18 @@ "bytes": 5032, "description": "BNGsim ExprTkEvaluator public wrapper header" }, + "expr_compat_header": { + "path": "third_party/bngsim_expr/include/bngsim/expr_compat.hpp", + "source_path": "include/bngsim/expr_compat.hpp", + "sha256": "00c6944497d5405c03a11d92c6295452963d70f304378e5be9561af58762ed7b", + "bytes": 2778, + "description": "BNGsim BNG<->ExprTk compatibility primitives (GH #49)" + }, "expression_source": { "path": "third_party/bngsim_expr/src/expression.cpp", "source_path": "src/expression.cpp", - "sha256": "4a7bf502cdd6be1e381e754140e35f0bf57a3fc9e778a5723db9b7909ca03a64", - "bytes": 33050, + "sha256": "9b54169c75e672e74fe6ca3825e5dcd027f7dee463821fc767991ccf775962fd", + "bytes": 36878, "description": "BNGsim ExprTkEvaluator implementation" } }, diff --git a/third_party/bngsim_expr/include/bngsim/expr_compat.hpp b/third_party/bngsim_expr/include/bngsim/expr_compat.hpp new file mode 100644 index 0000000..65f93e3 --- /dev/null +++ b/third_party/bngsim_expr/include/bngsim/expr_compat.hpp @@ -0,0 +1,54 @@ +// bngsim/include/bngsim/expr_compat.hpp — BNG ↔ ExprTk compatibility primitives +// +// Single source of truth for the BNG-compatibility logic shared by the two +// ExprTk-backed expression layers in BNGsim: +// +// * the host evaluator — bngsim::ExprTkEvaluator (src/expression.cpp), used +// by the ODE/SSA engines and NfsimSimulator::evaluate_expression; and +// * the vendored NFsim mu::Parser shim (third_party/nfsim/src/NFfunction/ +// nfsim_funcparser.h), which evaluates NFsim function columns in-process. +// +// Both layers must agree on (a) the BNGL built-in mratio(), and (b) how a model +// identifier is mapped onto an ExprTk-legal symbol name (the unconditional +// leading-underscore remap and the conditional reserved-symbol mangle). Before +// issue #49 each feature was hand-ported into both layers and kept in sync by +// guard tests; the shim now forwards to these definitions instead, so the logic +// lives in exactly one place (the bngsim::expression static archive that the +// vendored NFsim target links). See issue #49 and bngsim ADR-005 +// (bngsim/dev/adr/ADR-005-nfsim-exprtk-shim-single-source.md). +// +// The declarations here are deliberately free of any ExprTk dependency: the +// reserved-symbol set is built from exprtk.hpp inside the single translation +// unit that defines is_exprtk_reserved(), so neither the host call sites nor +// the NFsim shim need to include the 1.6 MB ExprTk header to call these. +#pragma once + +#include + +namespace bngsim { +namespace expr_compat { + +// mratio(a, b, z): the BNGL built-in confluent-hypergeometric ratio +// M(a+1, b+1, z) / M(a, b, z), evaluated by Gauss's continued fraction with the +// modified-Lentz method so it stays finite on BNG's large-|z| range (issue #42). +double mratio(double a, double b, double z); + +// Unconditional leading-underscore remap: "_X" → "u_X" (ExprTk rejects an +// identifier starting with '_'). Identity for any name not starting with '_'. +// No ExprTk built-in starts with '_', so this rewrite is always safe to apply +// token-by-token to an expression string. +std::string remap_name(const std::string &name); + +// True if `name`, registered as an ExprTk variable, would collide with a name +// the symbol table already owns: an ExprTk reserved word/symbol, a bngsim +// function alias (ln/rint/sign/mratio/time), or one of the "u_*" keys the +// built-in constants occupy after the underscore remap (GH #90). +bool is_exprtk_reserved(const std::string &name); + +// Symbol-table key for `name` at registration time: the unconditional +// underscore remap first, then the conditional reserved-symbol mangle +// ("X" → "r_X"). Identity if neither applies. +std::string compute_registration_name(const std::string &name); + +} // namespace expr_compat +} // namespace bngsim diff --git a/third_party/bngsim_expr/src/expression.cpp b/third_party/bngsim_expr/src/expression.cpp index 6ad4b3e..77697d1 100644 --- a/third_party/bngsim_expr/src/expression.cpp +++ b/third_party/bngsim_expr/src/expression.cpp @@ -19,6 +19,7 @@ #define exprtk_disable_caseinsensitivity #include "exprtk.hpp" +#include "bngsim/expr_compat.hpp" #include "bngsim/expression.hpp" #include @@ -62,7 +63,11 @@ namespace bngsim { // double's representable range — and the ratio becomes inf/inf = nan. Lentz // stays O(1) throughout and converges in a few hundred iterations even on // those inputs. -static double mratio_impl(double a, double b, double z) { +// +// This is the single source of truth for mratio across BNGsim: the host +// MratioFunction adapter below and the vendored NFsim mu::Parser ExprTk shim +// both call it (the shim via ). See issue #49. +double expr_compat::mratio(double a, double b, double z) { constexpr double eps = 1.0e-16; constexpr double tiny = 1.0e-32; // Safety cap so a pathological non-converging case fails loud rather @@ -204,7 +209,7 @@ template struct MratioFunction : public exprtk::ifunction { const double da = static_cast(a); const double db = static_cast(b); const double dz = static_cast(z); - const double r = mratio_impl(da, db, dz); + const double r = expr_compat::mratio(da, db, dz); if (warner) { warner->warn_if_nonfinite("mratio", {da, db, dz}, r); } @@ -376,7 +381,22 @@ static const std::unordered_set &bngsim_exprtk_aliases() { return aliases; } -static bool is_exprtk_reserved(const std::string &name) { +// Registration keys occupied by bngsim's built-in constants. init_builtins() +// registers each "_X" constant (Planck's `_h`, Avogadro's `_NA`, …) under the +// ExprTk key "u_X" because ExprTk rejects a leading '_'. A user parameter +// named literally "u_h" / "u_pi" / … maps to that same key (it does not start +// with '_', so compute_registration_name() would leave it unchanged) and +// would collide with the constant slot at registration (GH #90: Chitnis2012 +// / BIOMD0000000950 has a parameter `u_h`). Treat these keys as reserved so +// such names take the r_ mangling path like any other reserved-word collision. +// Must stay in sync with the "_X" constants registered in init_builtins(). +static const std::unordered_set &bngsim_remapped_constant_keys() { + static const std::unordered_set keys = {"u_pi", "u_e", "u_kB", "u_NA", + "u_R", "u_h", "u_F"}; + return keys; +} + +bool expr_compat::is_exprtk_reserved(const std::string &name) { // Names that, if registered as a user variable, would collide with // a name already taken by the symbol table. Two sources: // @@ -395,20 +415,36 @@ static bool is_exprtk_reserved(const std::string &name) { // the `sign` collision in practice — but we mangle all five // for symmetry and to handle hand-crafted .net inputs. // + // 3. The registration keys bngsim's built-in constants occupy after the + // unconditional "_X" → "u_X" remap (`u_pi`, …, `u_h`, `u_F`). A user + // parameter named literally `u_h` would otherwise alias Planck's + // constant slot and fail to register (GH #90). + // // Comparison is exact (case-sensitive) because we build with // exprtk_disable_caseinsensitivity, so e.g. `Const` is not reserved. const auto &exprtk_reserved = exprtk_reserved_identifiers(); const auto &bngsim_aliases = bngsim_exprtk_aliases(); + const auto &constant_keys = bngsim_remapped_constant_keys(); return exprtk_reserved.find(name) != exprtk_reserved.end() || - bngsim_aliases.find(name) != bngsim_aliases.end(); + bngsim_aliases.find(name) != bngsim_aliases.end() || + constant_keys.find(name) != constant_keys.end(); } -// Compute the symbol-table key for `name` at registration time. -// Combines the unconditional underscore remap with reserved-word mangling. -static std::string compute_registration_name(const std::string &name) { +// Unconditional leading-underscore remap "_X" → "u_X" (see expr_compat.hpp). +std::string expr_compat::remap_name(const std::string &name) { if (!name.empty() && name[0] == '_') { return "u_" + name.substr(1); } + return name; +} + +// Compute the symbol-table key for `name` at registration time. +// Combines the unconditional underscore remap with reserved-word mangling. +std::string expr_compat::compute_registration_name(const std::string &name) { + std::string underscore_mapped = remap_name(name); + if (underscore_mapped != name) { + return underscore_mapped; + } if (is_exprtk_reserved(name)) { return "r_" + name; } @@ -466,8 +502,9 @@ struct ExprTkEvaluator::Impl { // that were actually registered on this evaluator, so built-in tokens // (sin, if, time, ...) pass through unchanged. std::string remap_token(const std::string &name) const { - if (!name.empty() && name[0] == '_') { - return "u_" + name.substr(1); + std::string underscore_mapped = expr_compat::remap_name(name); + if (underscore_mapped != name) { + return underscore_mapped; } auto it = mangled_user_names.find(name); if (it != mangled_user_names.end()) { @@ -540,7 +577,37 @@ struct ExprTkEvaluator::Impl { (std::isalnum(static_cast(expr[i])) || expr[i] == '_')) { i++; } - result += remap_token(expr.substr(start, i - start)); + const std::string token = expr.substr(start, i - start); + // A declared symbol that collides with an ExprTk reserved name + // and is *also* used in call form (`frac(x)`) is genuinely + // ambiguous in a single flat namespace — raise the same clear, + // deterministic error the NFsim mu::Parser shim does, rather + // than rewriting to r_ and letting ExprTk emit a cryptic + // "not a function" message. strip_empty_parens() has already + // removed legitimate zero-arg scalar calls (`obs()`), so any + // `name(` left for a mangled symbol is a real call form. Only + // reserved-mangled names are tracked in mangled_user_names; + // underscore remaps are not, so they fall through untouched. + if (mangled_user_names.count(token)) { + size_t j = i; + while (j < expr.size() && std::isspace(static_cast(expr[j]))) { + j++; + } + if (j < expr.size() && expr[j] == '(') { + throw std::runtime_error( + "identifier '" + token + + "' is both a declared model " + "symbol and used as a function call '" + + token + + "(...)'; " + "ExprTk reserves '" + + token + + "' as a built-in and a single " + "flat namespace cannot hold both meanings — rename the " + "model symbol"); + } + } + result += remap_token(token); continue; } result += expr[i]; @@ -550,7 +617,7 @@ struct ExprTkEvaluator::Impl { } void add_remapped_constant(const std::string &name, double value) { - std::string mapped = compute_registration_name(name); + std::string mapped = expr_compat::compute_registration_name(name); if (mapped != name && name[0] != '_') { mangled_user_names[name] = mapped; } @@ -612,9 +679,9 @@ ExprTkEvaluator::ExprTkEvaluator() : impl_(std::make_unique()) {} ExprTkEvaluator::~ExprTkEvaluator() = default; void ExprTkEvaluator::define_variable(const std::string &name, double *addr) { - std::string mapped = compute_registration_name(name); + std::string mapped = expr_compat::compute_registration_name(name); if (!impl_->symbol_table.add_variable(mapped, *addr)) { - const bool reserved = is_exprtk_reserved(name); + const bool reserved = expr_compat::is_exprtk_reserved(name); std::string detail; if (reserved) { // The mangled form already exists — most often because another