Render expressions via AST rewrites - #50
Merged
Merged
Conversation
formula_to_string ran formulaToL3String and then patched the rendered string with regexes and a token loop to fix integer division and swap function/constant names. Matching names by spelling had no variable-vs-function guard (that guard existed only for constants), so a model variable spelled like a C++ function (min, max, sin, ...) was silently rewritten into that function, producing wrong C++. Keep formulaToL3String as the sole renderer (precedence, parenthesisation and number formatting are untouched) and map the required changes onto the AST instead, by node kind: - resolve_constants renames symbol and non-finite nodes to their C++ text - map_functions handles power/root/log, degenerate plus/times, n-ary relationals, logical operators (renamed in place to keep libsbml's infix-vs-call choice), and the std::/sm:: name maps Delete the token loop, rewrite_power, rewrite_nary_relational and the placeholder machinery; only the integer->double regex remains. formula_to_string's signature is unchanged. Output is byte-identical across all reference models and the full SBML test suite (1821 cases); the one behaviour change is fixing the latent variable-vs-function-name collision above. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors SBML expression rendering to rewrite typed AST nodes before libSBML generates C++.
Changes:
- Maps constants, functions, powers, roots, logs, and operators through AST rewrites.
- Adds broad rendering regression and edge-case coverage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
chaste_sbml/_expressions.py |
Implements AST-driven C++ expression mapping. |
chaste_sbml/tests/test_expressions.py |
Expands expression-rendering coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A call to a model's own function definition is a generic AST_FUNCTION node that also reports isFunction(), so the name-map branch was rerouting a model function spelled like a built-in (sin, min, log) to std::/sm::, calling the wrong implementation. Restrict the name maps to libsbml's typed built-in function nodes and leave generic AST_FUNCTION calls unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Fixes #49