-
-
Notifications
You must be signed in to change notification settings - Fork 22
SymEngine #1357
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SymEngine #1357
Changes from all commits
af16cf6
9e2b0b3
dafb56e
4331798
3953282
2de9098
1238673
539dbf7
f2cab14
bdb3bd1
3b2c5cc
060b815
685e1a1
81f5766
3e8456d
cb8b4ee
9020f40
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -183,6 +183,13 @@ if(NOT DEFINED _ZLIB_FIND_REPORTED) | |||||||||||||||||||||||||
| message(STATUS "Found ZLIB: ${ZLIB_LIBRARIES} (found version \"${ZLIB_VERSION_STRING}\").") | ||||||||||||||||||||||||||
| endif() | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Set the minimum policy version to work around SymEngine's outdated CMake requirements. | ||||||||||||||||||||||||||
| set(CMAKE_POLICY_VERSION_MINIMUM 3.10) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| # Find SymEngine. | ||||||||||||||||||||||||||
| find_package(SymEngine REQUIRED CONFIG) | ||||||||||||||||||||||||||
| message(STATUS "Found SymEngine: ${SYMENGINE_LIBRARIES} (found version \"${SymEngine_VERSION}\").") | ||||||||||||||||||||||||||
|
Comment on lines
+187
to
+191
|
||||||||||||||||||||||||||
| set(CMAKE_POLICY_VERSION_MINIMUM 3.10) | |
| # Find SymEngine. | |
| find_package(SymEngine REQUIRED CONFIG) | |
| message(STATUS "Found SymEngine: ${SYMENGINE_LIBRARIES} (found version \"${SymEngine_VERSION}\").") | |
| cmake_policy(PUSH) | |
| cmake_policy(VERSION 3.10) | |
| # Find SymEngine. | |
| find_package(SymEngine REQUIRED CONFIG) | |
| message(STATUS "Found SymEngine: ${SYMENGINE_LIBRARIES} (found version \"${SymEngine_VERSION}\").") | |
| cmake_policy(POP) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /* | ||
| Copyright libOpenCOR contributors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| #ifdef __clang__ | ||
| # pragma clang diagnostic push | ||
| # pragma clang diagnostic ignored "-Wdeprecated-literal-operator" | ||
| # pragma clang diagnostic ignored "-Wunused-parameter" | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /* | ||
| Copyright libOpenCOR contributors. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| #ifdef __clang__ | ||
| # pragma clang diagnostic pop | ||
| #endif |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,10 @@ limitations under the License. | |||||||||||
| #include <cmath> | ||||||||||||
| #include <iterator> | ||||||||||||
|
|
||||||||||||
| #include "symenginebegin.h" | ||||||||||||
| #include <symengine/solve.h> | ||||||||||||
| #include "symengineend.h" | ||||||||||||
|
|
||||||||||||
| #include "libcellml/analyserequation.h" | ||||||||||||
| #include "libcellml/analyserexternalvariable.h" | ||||||||||||
| #include "libcellml/generatorprofile.h" | ||||||||||||
|
|
@@ -198,6 +202,133 @@ bool AnalyserInternalEquation::variableOnLhsOrRhs(const AnalyserInternalVariable | |||||||||||
| || variableOnRhs(variable); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| SymEngineEquationResult AnalyserInternalEquation::symEngineEquation(const AnalyserEquationAstPtr &ast, const SymEngineSymbolMap &symbolMap) | ||||||||||||
| { | ||||||||||||
| if (ast == nullptr) { | ||||||||||||
| return {true, SymEngine::null}; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| AnalyserEquationAstPtr leftAst = ast->leftChild(); | ||||||||||||
| AnalyserEquationAstPtr rightAst = ast->rightChild(); | ||||||||||||
|
|
||||||||||||
| // Recursively call getConvertedAst on left and right children. | ||||||||||||
|
||||||||||||
| // Recursively call getConvertedAst on left and right children. | |
| // Recursively call symEngineEquation on left and right children. |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()) { |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parseSymEngineExpression is called with 'nullptr' as the parent for the rearrangedEquationAst, but then the parent is set to 'ast' on line 327. This is inconsistent - the parent should either be passed during creation or set immediately after. The current approach creates a temporary inconsistent state.
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.