Extended Rearrangement#3
Merged
Merged
Conversation
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
agarny
requested changes
Dec 17, 2025
Collaborator
agarny
left a comment
agarny
left a comment
There was a problem hiding this comment.
- In your tests, in addition to confirming that a model is of algebraic type, it would be good to confirm that its equation looks the way we would expect. For this, you could use
Generator::equationCode(). - No need to have our CellML files named
unarranged_xxx.cellml. They can simply be namedxxx.cellmlsince they are located under thesymenginefolder. - For the different equations you are testing, it would be nice to have a comment on why they are being tested.
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
RLee64
commented
Dec 17, 2025
Owner
Author
RLee64
left a comment
RLee64
left a comment
There was a problem hiding this comment.
In your tests, in addition to confirming that a model is of algebraic type, it would be good to confirm that its equation looks the way we would expect. For this, you could use Generator::equationCode().
No need to have our CellML files named unarranged_xxx.cellml. They can simply be named xxx.cellml since they are located under the symengine folder.
For the different equations you are testing, it would be nice to have a comment on why they are being tested.
In addition to these changes I've also done the following
- Cleaned up the trigonometric test case so it's only testing for functionality we know will pass in symengine's current state (i.e. no operations needed to inverse functions). d79d8fc
- Fixed an issue where unary functions weren't being properly converted back (this wasn't picked up since we hadn't encountered symengine outputting unary functions until the proper trigonometric test case) aab7d89
- Fixed issue where ast children weren't being assigned their parents (this one I'm just embarrased I hadn't noticed sooner) 9c1e0b8
x + (-y) now becomes x - y -1 * y now becomes -y this also means x + (-1 * y) becomes x - y
agarny
approved these changes
Dec 18, 2025
Collaborator
agarny
left a comment
agarny
left a comment
There was a problem hiding this comment.
Approving, but... at some point, we will need to ensure that ALL tests pass, incl. coverage tests, memory checks, etc.
This was referenced Dec 18, 2025
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
2 participants
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.
Overview
In #1 we resolved the rearrangement example seen in cellml#1354 for the trivial x = y + z case. However, we want to expand upon this functionality in order to support more operations. As such, this pull request focuses on growing the existing level of support for equation rearrangement, expanding functionality to other use cases. That being said, this implementation does not fully cover all libcellml supported types, but we hope to expand upon this in an additional pull request.
Here we also begin to build a set of proper test cases for us to ensure SymEngine's rearrangement functionality is behaving as we intend.
Changes
isSymEngineExpressionComplexwas introduced to recursively look for complex constituents. This is consequently used to remove complex equations from our solution set.Notes