SymEngine #1357
SymEngine #1357
Conversation
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
|
Argh, I targeted the wrong repo, so closing! |
There was a problem hiding this comment.
Pull request overview
This PR integrates the SymEngine library to enable automatic rearrangement of algebraic equations. This allows the analyser to handle equations where variables are not explicitly isolated, improving the robustness of model analysis.
Key changes:
- Adds SymEngine dependency for symbolic equation manipulation
- Implements equation rearrangement functionality in the analyser
- Removes DAE model test files and associated test case
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/analyser_p.h | Forward declarations and type aliases for SymEngine integration |
| src/analyser.cpp | Implementation of equation rearrangement using SymEngine solve functionality |
| src/3rdparty/symengine/symenginebegin.h | Compiler diagnostic suppression for SymEngine headers (Clang) |
| src/3rdparty/symengine/symengineend.h | Restore compiler diagnostic settings after SymEngine includes |
| src/CMakeLists.txt | Links SymEngine library and adds include directories |
| cmake/environmentchecks.cmake | Adds SymEngine package detection |
| .github/workflows/ci.yml | Installs SymEngine in CI workflows for all platforms |
| tests/analyser/analyser.cpp | Adds test for algebraic equation rearrangement |
| tests/resources/analyser/unarranged_algebraic_equation.cellml | Test model with equation requiring rearrangement |
| tests/generator/generator.cpp | Removes DAE model test case |
| tests/resources/generator/dae_cellml_1_1_model/model.{py,h,c,cellml} | Removes DAE test model files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // TODO Update to account for symengine expressions with 3 or more children. | ||
| if (children.size() > 0) { |
There was a problem hiding this comment.
The comparison 'children.size() > 0' is less idiomatic than 'children.empty()' or '!children.empty()'. Using the empty() method is clearer and is the standard C++ way to check if a container is non-empty.
| if (children.size() > 0) { | |
| if (!children.empty()) { |
| SymEngineSymbolMap symbolMap; | ||
| SymEngineVariableMap variableMap; | ||
|
|
||
| for (const auto &variable : mAllVariables) { |
There was a problem hiding this comment.
The parameter name 'variable' shadows the loop variable 'variable' used in the for-loop. This makes the code confusing and could lead to errors. The parameter should be renamed to something like 'targetVariable' or 'variableToIsolate'.
| // TODO Update to account for symengine expressions with 3 or more children. | ||
| if (children.size() > 0) { | ||
| ast->setLeftChild(parseSymEngineExpression(children[0], ast, variableMap)); | ||
| if (children.size() > 1) { | ||
| ast->setRightChild(parseSymEngineExpression(children[1], ast, variableMap)); | ||
| } |
There was a problem hiding this comment.
The TODO indicates that expressions with 3 or more children are not properly handled. However, operations like addition and multiplication are commonly n-ary in mathematical expressions (e.g., a + b + c). The current implementation only processes the first two children and silently ignores the rest, which will produce incorrect results for such expressions.
| return {false, SymEngine::null}; | ||
| } | ||
|
|
||
| // Analyse mAst current type and value. |
There was a problem hiding this comment.
The comment refers to "mAst" but should refer to "ast" which is the parameter. The current variable being analyzed is the parameter, not the member variable.
| // Analyse mAst current type and value. | |
| // Analyse ast current type and value. |
| default: | ||
| break; | ||
| } |
There was a problem hiding this comment.
The default case in the switch statement silently breaks without setting any type for the AST node. This leaves the AST in an incomplete state when encountering unsupported SymEngine expression types, which could lead to undefined behavior when the AST is later used.
| - name: Configure libCellML | ||
| run: | | ||
| emcmake cmake -G Ninja -S . -B build-wasm -DBUILD_TYPE=Release -DLIBXML2_INCLUDE_DIR=$HOME/libxml2/include/libxml2 -DLIBXML2_LIBRARY=$HOME/libxml2/lib/libxml2.a -DZLIB_INCLUDE_DIR=$HOME/zlib/include -DZLIB_LIBRARY=$HOME/zlib/lib/libz.a | ||
| emcmake cmake -G Ninja -S . -B build-wasm -DBUILD_TYPE=Release -DLIBXML2_INCLUDE_DIR=$HOME/libxml2/include/libxml2 -DLIBXML2_LIBRARY=$HOME/libxml2/lib/libxml2.a -DSYMENGINE_INCLUDE_DIR=$HOME/symengine/include -DSYMENGINE_LIBRARY=$HOME/symengine/lib/libsymengine.a -DZLIB_INCLUDE_DIR=$HOME/zlib/include -DZLIB_LIBRARY=$HOME/zlib/lib/libz.a |
There was a problem hiding this comment.
The WASM build configuration uses SYMENGINE_INCLUDE_DIR and SYMENGINE_LIBRARY directly, but the find_package(SymEngine REQUIRED CONFIG) in cmake/environmentchecks.cmake expects SymEngine to be found via CMake config mode. This inconsistency means the WASM build bypasses the standard SymEngine detection mechanism, which could lead to issues if the variable names or detection logic changes.
| emcmake cmake -G Ninja -S . -B build-wasm -DBUILD_TYPE=Release -DLIBXML2_INCLUDE_DIR=$HOME/libxml2/include/libxml2 -DLIBXML2_LIBRARY=$HOME/libxml2/lib/libxml2.a -DSYMENGINE_INCLUDE_DIR=$HOME/symengine/include -DSYMENGINE_LIBRARY=$HOME/symengine/lib/libsymengine.a -DZLIB_INCLUDE_DIR=$HOME/zlib/include -DZLIB_LIBRARY=$HOME/zlib/lib/libz.a | |
| emcmake cmake -G Ninja -S . -B build-wasm -DBUILD_TYPE=Release -DCMAKE_PREFIX_PATH=$HOME/symengine -DLIBXML2_INCLUDE_DIR=$HOME/libxml2/include/libxml2 -DLIBXML2_LIBRARY=$HOME/libxml2/lib/libxml2.a -DZLIB_INCLUDE_DIR=$HOME/zlib/include -DZLIB_LIBRARY=$HOME/zlib/lib/libz.a |
| auto newAst = rearrangeFor(unknownVariableLeft); | ||
| if (newAst != nullptr) { | ||
| // TODO Update variables and/or equation type when necessary. | ||
| mAst = newAst; |
There was a problem hiding this comment.
When rearrangement fails (newAst is nullptr), the code silently continues with the original AST. However, the subsequent check on line 421-422 still requires the variable to be on LHS or RHS, which will fail. This means equations that can't be rearranged will incorrectly fail analysis, potentially marking variables as overconstrained or underconstrained when they're simply not rearrangeable with the current implementation.
| mAst = newAst; | |
| mAst = newAst; | |
| } else { | |
| // Rearrangement failed, so we no longer treat this variable as isolated. | |
| unknownVariableLeft = nullptr; |
| AnalyserEquationAstPtr leftAst = ast->leftChild(); | ||
| AnalyserEquationAstPtr rightAst = ast->rightChild(); | ||
|
|
||
| // Recursively call getConvertedAst on left and right children. |
There was a problem hiding this comment.
The comment refers to "getConvertedAst" but the function being called is "symEngineEquation". This appears to be a copy-paste error or outdated comment from refactoring.
| // Recursively call getConvertedAst on left and right children. | |
| // Recursively call symEngineEquation on left and right children. |
| switch (ast->type()) { | ||
| case AnalyserEquationAst::Type::EQUALITY: | ||
| return {true, Eq(left, right)}; | ||
| case AnalyserEquationAst::Type::PLUS: | ||
| return {true, add(left, right)}; | ||
| case AnalyserEquationAst::Type::CI: | ||
| // Seems like the voi doesn't exist in mAllVariables, so we don't have an easy means of access. | ||
| if (symbolMap.find(ast->variable()->name()) == symbolMap.end()) { | ||
| return {false, SymEngine::null}; | ||
| } | ||
| return {true, symbolMap.at(ast->variable()->name())}; | ||
| default: | ||
| // Rearrangement is not possible with this type. | ||
| return {false, SymEngine::null}; | ||
| } |
There was a problem hiding this comment.
The switch statement only handles EQUALITY, PLUS, and CI types, returning false for all other types. However, basic mathematical operations like subtraction, multiplication, division, and exponentiation are likely needed for equation rearrangement. The current implementation will fail to rearrange equations containing these operations.
| switch (ast->type()) { | ||
| case AnalyserEquationAst::Type::EQUALITY: | ||
| return {true, Eq(left, right)}; | ||
| case AnalyserEquationAst::Type::PLUS: | ||
| return {true, add(left, right)}; | ||
| case AnalyserEquationAst::Type::CI: | ||
| // Seems like the voi doesn't exist in mAllVariables, so we don't have an easy means of access. | ||
| if (symbolMap.find(ast->variable()->name()) == symbolMap.end()) { | ||
| return {false, SymEngine::null}; | ||
| } | ||
| return {true, symbolMap.at(ast->variable()->name())}; | ||
| default: | ||
| // Rearrangement is not possible with this type. | ||
| return {false, SymEngine::null}; | ||
| } |
There was a problem hiding this comment.
The parseSymEngineExpression function handles MUL (multiplication) type but the symEngineEquation function does not convert TIMES to multiplication when building the SymEngine expression. This asymmetry means the conversion to SymEngine will fail for equations containing multiplication, even though the reverse conversion is implemented.
2 participants
No description provided.