Strengthen alias elimination #1635
Merged
+122
−31
Conversation
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
YingboMa
reviewed
Jun 9, 2022
| may_eliminate &= isnothing(diff_to_var[v]) && isnothing(var_to_diff[v]) | ||
| end | ||
| locally_structure_simplify!((@view mm[ei, :]), vi, ag, may_eliminate) | ||
| locally_structure_simplify!((@view mm[ei, :]), vi, ag, var_to_diff) |
There was a problem hiding this comment.
Could you make sure that variables that are marked irreducible to not be eliminated away? See
There was a problem hiding this comment.
irreducible values are not included in the linear subsystem, so they should not be eliminated.
|
BTW, remember to run |
|
The CI failures https://github.com/SciML/ModelingToolkit.jl/runs/6804925627?check_suite_focus=true#step:6:1540 look legitimate. Are you sure they still eliminate the same number of variables as the number of equations to keep the system balanced? |
Previously, we would only alias eliminate variables that do not occur differentiated. This was unnecessarily restricted for systems such as ``` x ~ 0 D(x) ~ x - y ``` where alias elimination is now able to determine that `x` and `y` are aliases. While this is the main point of the PR, this does improve alias elimination in some of the tests, causing tearing order to change. Previously we had a suboptimality in tearing where an equation assigned to an scc in which it is not able to solve a variable would not be reconsidered in another scc. Fix that also, to make sure the tests keep working.
YingboMa
reviewed
Jun 11, 2022
| @@ -37,17 +37,57 @@ end | |||
|
|
|||
| function tear_graph_modia(structure::SystemStructure; varfilter = v -> true, | |||
| eqfilter = eq -> true) | |||
| # It would be possible here to simply iterate over all variables and attempt to | |||
There was a problem hiding this comment.
Should we split this to another PR?
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.
Previously, we would only alias eliminate variables that do not occur
differentiated. This was unnecessarily restricted for systems such as
where alias elimination is now able to determine that
xandyarealiases.
While this is the main point of the PR, this does improve alias elimination
in some of the tests, causing tearing order to change. Previously we had
a suboptimality in tearing where an equation assigned to an scc in which it
is not able to solve a variable would not be reconsidered in another scc.
Fix that also, to make sure the tests keep working.