-
Notifications
You must be signed in to change notification settings - Fork 518
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
Improve performance of solve_strongly_connected_components
for models with named expressions
#3186
Conversation
… in solve_strongly_connected_components
…f one already exists
…tion of identify_external_functions to be a generator
solve_strongly_connected_components
for models with large expression treessolve_strongly_connected_components
for models with named expressions
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.
Looks good. A couple questions / suggestions, but nothing that would prevent merging
FYI: Jenkins test failures appear to be real:
|
Real failure again, @Robbybp
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3186 +/- ##
==========================================
- Coverage 88.39% 88.39% -0.01%
==========================================
Files 846 846
Lines 95091 95118 +27
==========================================
+ Hits 84058 84079 +21
- Misses 11033 11039 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
My latest commits should address this. |
Summary/Motivation:
Working with a small IDAES model, I noticed that
solve_strongly_connected_components
was much slower than I would have expected. This is for a model with about 900 variables and constraints:solve_strongly_connected_components
takes 40 s, half of which is incalculate_variable_from_constraint
. This prompted me to review a lot of the infrastructure thatsolve_strongly_connected_components
relies on and look for ways to speed things up. Most of the performance gains I will demonstrate come from not repeating work for the same named expressions, although reusing incidence graphs instead of walking expressions helps as well.Changes proposed in this PR:
solve_strongly_connected_components
to avoid usingcalculate_variable_from_constraint
. This can be beneficial for models with large, nested named expressions, which the NL writer exploits butcalculate_variable_from_constraint
does not.IncidenceMethod.ampl_repn
when constructing incidence graphs. Again, this exploits named expressions, whereIncidenceMethod.standard_repn
does not.generate_strongly_connected_components
_ExternalFunctionVisitor
andadd_local_external_functions
to exploit named expressionsTemporarySubsystemManager
to temporarily remove bounds on fixed variables. This was the motivation for the suggestion in UnavoidableInfeasibleConstraintException
in NL writer for variable fixed outside of bounds #3179The timing breakdown after these improvements looks like:
We are now bottlenecked by
identify_variables
, of all things.Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: