CMake: add support for SymEngine #1358

agarny · 2025-12-18T21:15:38Z

No description provided.

Also changed the name of astMap to variableMap to better reflect what it's actually mapping to

Substituted with std::pair<bool, SymEngine::RCP<const SymEngine::Basic>> use instead

Mostly GPT generated test cases

Test case has been superseded by the newly added test suite to more robustly test for equation rearrangement

Previous system assumed that symengine expressions contained only 0-2 children, but in reality it could be any whole number. This adjusts the existing conversion logic to account for this.

Made it so that we also now consider mathml unary +/- operators

Can now handle unary plus, minus, unary minus, and CN types from AST (which means code now passes Analyser.rearrangeAdditiveEquations)

Had an error at equation 4 Also added brackets to equation comments to make things more clear

Now passes Analyser.rearrangeMultiplicativeEquations

Now covers a greater variety of cases

Symengine is not capable of rearranging trig functions into inverse trig functions (e.g. tan(x) = y cannot become x = atan(y))

Now has better Eq 1 test case and more consistent variable naming

Now supports - Decimal point values (i.e. doubles) - Constants (E, pi, and inf)

We need to ensure that there can only be one real solution for each

Some specific things had to be added to handle this - CN representing integers must be appropriately converted since symengine requires exponentials to be whole numbers - We need to filter out real from non-real solutions since we assume users will want to ignore the complex domain in most cases

Now assumes left child will always be the non-null component

I can't believe I didn't get an error for this sooner!!!

- Added comments - Tried to give each test case a more 'unique' purpose

Also moved initialising constants to the bottom of the mathml block so we guarantee that our tested equation range always starts at 0

x + (-y) now becomes x - y -1 * y now becomes -y this also means x + (-1 * y) becomes x - y

agarny · 2025-12-18T21:16:00Z

Argh, wrong repo!

Copilot

Pull request overview

This pull request adds support for the SymEngine symbolic mathematics library to enable automatic equation rearrangement in the libCellML analyser. The integration allows the analyser to solve for unknowns in algebraic equations that are not already in the form variable = expression, eliminating the need for numerical equation solvers in some cases.

Key changes:

Integration of SymEngine library for symbolic equation manipulation and solving
New equation rearrangement capabilities in the analyser
Addition of comprehensive test cases for various equation types (trigonometric, polynomial, multiplicative, etc.)

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
cmake/environmentchecks.cmake	Adds SymEngine package detection and configuration
src/CMakeLists.txt	Links SymEngine libraries and includes directories for the cellml target
src/analyser_p.h	Adds SymEngine-related type definitions and forward declarations for new equation rearrangement methods
src/analyser.cpp	Implements equation conversion to/from SymEngine format, symbolic solving, and AST reconstruction
src/3rdparty/symengine/symenginebegin.h	Pragma directives to suppress SymEngine compiler warnings on Clang
src/3rdparty/symengine/symengineend.h	Pragma directives to restore compiler warning settings after SymEngine includes
tests/analyser/tests.cmake	Adds new SymEngine test file to the build configuration
tests/analyser/analysersymengine.cpp	Comprehensive test suite covering addition, multiplication, trigonometric functions, constants, and polynomial equations
tests/resources/analyser/symengine/*.cellml	Test model files for validating equation rearrangement functionality
tests/generator/generator.cpp	Removes DAE model test (likely now handled by SymEngine rearrangement)
tests/resources/generator/dae_cellml_1_1_model/*	Removes DAE model test resources
.github/workflows/ci.yml	Adds GMP and SymEngine installation steps for all platforms, updates to staging branch trigger, modernizes CMake invocations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-18T21:21:25Z

src/analyser.cpp

+    auto currentAst = ast;
+
+    // All children (except the last) are guaranteed to be left children in the AST tree.
+    for (int i = 0; i + 1 < children.size(); ++i) {


The loop variable i is declared as int but is being compared with children.size() which returns size_t (an unsigned type). This causes a signed/unsigned comparison warning. Use size_t for the loop variable instead to avoid potential issues with large collections and eliminate compiler warnings.

Copilot · 2025-12-18T21:21:25Z

src/analyser.cpp

+        if (SymEngine::eq(*constant, *SymEngine::E)) {
+            ast->setType(AnalyserEquationAst::Type::E);
+        } else if (SymEngine::eq(*constant, *SymEngine::pi)) {
+            ast->setType(AnalyserEquationAst::Type::PI);


When SymEngine::SYMENGINE_CONSTANT is encountered but the constant is neither E nor pi, the AST type is never set, leaving it in an undefined state. This will lead to incorrect behavior. Consider setting a default type, returning an error, or handling all possible SymEngine constants explicitly.

Suggested change

ast->setType(AnalyserEquationAst::Type::PI);

ast->setType(AnalyserEquationAst::Type::PI);

} else {

ast->setType(AnalyserEquationAst::Type::CN);

ast->setValue(seExpression->__str__());

Copilot · 2025-12-18T21:21:26Z

src/analyser.cpp

+    default:
+        break;
+    }


The default case at line 368 doesn't set any AST type, which will leave the AST in an undefined state when an unhandled SymEngine expression type is encountered. This could lead to silent failures or incorrect behavior. Consider logging an error, returning nullptr, or throwing an exception to indicate that the expression type is not supported.

Copilot · 2025-12-18T21:21:26Z

tests/resources/analyser/symengine/polynomials.cellml

+        <apply>
+          <apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>
+        </apply>


The XML structure is malformed. The power element at line 64 is missing its second child (the exponent). The apply element should contain both the base and the exponent as children. This will likely cause parsing errors. The structure should be: <apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>.

Suggested change

<apply>

<apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>

</apply>

<apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>

Copilot · 2025-12-18T21:21:26Z

src/analyser.cpp

+    AnalyserEquationAstPtr leftAst = ast->leftChild();
+    AnalyserEquationAstPtr rightAst = ast->rightChild();
+
+    // Recursively call getConvertedAst on left and right children.


The comment at line 216 mentions "getConvertedAst" but the actual function being called is "symEngineEquation". Update the comment to reference the correct function name to avoid confusion.

Suggested change

// Recursively call getConvertedAst on left and right children.

// Recursively call symEngineEquation on left and right children.

Copilot · 2025-12-18T21:21:28Z

src/analyser.cpp

+    for (const auto &variable : mAllVariables) {
+        SymEngine::RCP<const SymEngine::Symbol> symbol = SymEngine::symbol(variable->mVariable->name());
+        symbolMap[variable->mVariable->name()] = symbol;
+        variableMap[symbol] = variable;


The variable name variable shadows the function parameter with the same name. This makes the code confusing and error-prone. Consider renaming the loop variable to something like var or currentVariable to avoid the shadowing.

Suggested change

for (const auto &variable : mAllVariables) {

SymEngine::RCP<const SymEngine::Symbol> symbol = SymEngine::symbol(variable->mVariable->name());

symbolMap[variable->mVariable->name()] = symbol;

variableMap[symbol] = variable;

for (const auto &currentVariable : mAllVariables) {

SymEngine::RCP<const SymEngine::Symbol> symbol = SymEngine::symbol(currentVariable->mVariable->name());

symbolMap[currentVariable->mVariable->name()] = symbol;

variableMap[symbol] = currentVariable;

Copilot · 2025-12-18T21:21:28Z

tests/resources/analyser/symengine/polynomials.cellml

+        <apply>
+          <apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>
+        </apply>


There's an extra nested apply tag that doesn't have an operator. Line 63 shows <apply> followed immediately by another <apply><power/>... on line 64, but line 63's apply has no operator specified. This creates malformed MathML. Remove the extra apply tag on line 63.

Suggested change

<apply>

<apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>

</apply>

<apply><power/><ci>d</ci><cn cellml:units="dimensionless">3</cn></apply>

Copilot · 2025-12-18T21:21:28Z

src/analyser.cpp

+    }
+
+    // The final child is created and placed where appropriate.
+    if (children.size() != 0) {


The comparison children.size() != 0 should use the more idiomatic !children.empty() for checking if a container is empty. This is the preferred C++ style and may have better performance characteristics for some containers.

Suggested change

if (children.size() != 0) {

if (!children.empty()) {

Copilot · 2025-12-18T21:21:29Z

src/3rdparty/symengine/symenginebegin.h

@@ -0,0 +1,22 @@
+/*
+Copyright libOpenCOR contributors.


The copyright notice attributes the code to "libOpenCOR contributors" but this is a libCellML repository. The copyright should be "libCellML Contributors" to be consistent with the rest of the codebase, as seen in other files like tests/analyser/analysersymengine.cpp.

Suggested change

Copyright libOpenCOR contributors.

Copyright libCellML Contributors.

Copilot · 2025-12-18T21:21:29Z

src/3rdparty/symengine/symengineend.h

@@ -0,0 +1,19 @@
+/*
+Copyright libOpenCOR contributors.


The copyright notice attributes the code to "libOpenCOR contributors" but this is a libCellML repository. The copyright should be "libCellML Contributors" to be consistent with the rest of the codebase.

RLee64 and others added 30 commits December 12, 2025 13:26

Hardcoded installation of symengine and gmp

af16cf6

Created basic rearrangement test case

9e2b0b3

Basic implementation of equation rearrangement

dafb56e

Change symEngineRepresentation to symEngineEquation

4331798

Added TODO comments

3953282

Renamed equation to seEquation

2de9098

Defined type aliases for symengine maps

1238673

Also changed the name of astMap to variableMap to better reflect what it's actually mapping to

Updated function parameters to use const and pass by reference

539dbf7

Added support for parsing SymEngine integers to AST

f2cab14

Assign parenthood to recreated ast tree

bdb3bd1

Removed DAE test case and associated files

3b2c5cc

Replaced exception use in symEngineEquation()

060b815

Substituted with std::pair<bool, SymEngine::RCP<const SymEngine::Basic>> use instead

Removed redundant map include

685e1a1

Change SymEngineEquationResult to use tuple instead of pair

81f5766

Added rearrangement test suite

5dc3fcb

Mostly GPT generated test cases

Create new test cases for rearrangement

4d4ba97

Remove old rearrangement test case

d9c4d10

Test case has been superseded by the newly added test suite to more robustly test for equation rearrangement

Simple Equation Rearrangement.

3e8456d

Make SymEngine -> AST parsing work with 3+ children

bd67b0c

Previous system assumed that symengine expressions contained only 0-2 children, but in reality it could be any whole number. This adjusts the existing conversion logic to account for this.

Updated unarranged addition cellml file

c8d2bd8

Made it so that we also now consider mathml unary +/- operators

Add/Sub symengine parsing support

080d5c3

Can now handle unary plus, minus, unary minus, and CN types from AST (which means code now passes Analyser.rearrangeAdditiveEquations)

Fixed unarranged multiplication cellml file

cce543f

Had an error at equation 4 Also added brackets to equation comments to make things more clear

Mult/div symengine parsing support

41e85ce

Now passes Analyser.rearrangeMultiplicativeEquations

Improved unarranged trig test cases

4cbc787

Now covers a greater variety of cases

Simple trig rearrangement support

a47ce0d

Symengine is not capable of rearranging trig functions into inverse trig functions (e.g. tan(x) = y cannot become x = atan(y))

Updated unarranged constants cellml file

e157f82

Now has better Eq 1 test case and more consistent variable naming

Added support for various numbers and constants

4fdae53

Now supports - Decimal point values (i.e. doubles) - Constants (E, pi, and inf)

Fix unarranged polynomial test cases

339a003

We need to ensure that there can only be one real solution for each

Update unary +/- parsing

5d9cd2f

Now assumes left child will always be the non-null component

RLee64 and others added 16 commits December 17, 2025 17:18

Made explicit types auto

3a553be

Deleted unused test cases

9efff06

Renamed symengine cellml test files

f8a39f5

Cleaned up symengine trigonometric test case

d79d8fc

Fix issue with recreating unary functions for AST

aab7d89

Fixed children not setting their parent AST

9c1e0b8

I can't believe I didn't get an error for this sooner!!!

Cleaned symengine test cases

fc1cf1c

- Added comments - Tried to give each test case a more 'unique' purpose

Test expected generator equation code

ec0ace2

Also moved initialising constants to the bottom of the mathml block so we guarantee that our tested equation range always starts at 0

Made childSeExpression retrieval inline

f50049e

Fixed spelling error in symengine test files

5ca91b3

Removed redundant symengine multiplication test case

156bd52

Manual simplification of symengine results

ef336d5

x + (-y) now becomes x - y -1 * y now becomes -y this also means x + (-1 * y) becomes x - y

Extended rearrangement.

7c34ff2

CI: temporarily allow CI tests to be run on PRs to the staging branch.

a6005dd

CMake: find and use SymEngine.

9b686da

CI: use SymEngine as a dependency.

192b717

Copilot AI review requested due to automatic review settings December 18, 2025 21:15

agarny closed this Dec 18, 2025

Copilot started reviewing on behalf of agarny December 18, 2025 21:16 View session

Copilot AI reviewed Dec 18, 2025

View reviewed changes

-            ast->setType(AnalyserEquationAst::Type::PI);
+            ast->setType(AnalyserEquationAst::Type::PI);
+        } else {
+            ast->setType(AnalyserEquationAst::Type::CN);
+            ast->setValue(seExpression->__str__());

	// Recursively call getConvertedAst on left and right children.
	// Recursively call symEngineEquation on left and right children.

	Copyright libOpenCOR contributors.
	Copyright libCellML Contributors.

Uh oh!

CMake: add support for SymEngine #1358

CMake: add support for SymEngine #1358

Uh oh!

Conversation

agarny commented Dec 18, 2025

Uh oh!

agarny commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants