-
Notifications
You must be signed in to change notification settings - Fork 562
Add Primal/Dual Integral in MindtPy #2285
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
Conversation
bernalde
left a comment
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.
Hi Zedong, great first pass. We need to add also tests for the new functionality. Consider a list of primal and dual bound progress and compute the value of the integral as a test.
This could also be the change to add more unit tests to other pieces of MindtPy, especially cut_generation
Great work as always
| # TODO determine solve_data.LB, solve_data.UB is inf or -inf. | ||
| 'Final bound values: Primal Bound: {} Dual Bound: {}'. | ||
| format(solve_data.primal_bound, solve_data.dual_bound)) | ||
| # TODO determine solve_data.primal_bound, solve_data.dual_bound is inf or -inf. |
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.
Is this TODO still active or can we remove it?
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.
I'm not sure. Is there a case where cycling happens and no feasible solution has been found?
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.
No, cycling requires you to go back to the previously found solution.
| else: | ||
| solve_data.UB_progress.append(solve_data.UB) | ||
| # TODO: can we remove the following line? | ||
| # solve_data.dual_bound_progress.append(solve_data.dual_bound) |
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.
At the end is the dual_bound_progress complete? If so, we can remove it.
Codecov Report
@@ Coverage Diff @@
## main #2285 +/- ##
==========================================
- Coverage 85.29% 85.24% -0.06%
==========================================
Files 611 617 +6
Lines 76025 77932 +1907
==========================================
+ Hits 64849 66431 +1582
- Misses 11176 11501 +325
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
bernalde
left a comment
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.
I left behind some comments and we are still missing a little more tests. I'm reviewing this almost real-time but no pressure!
bernalde
left a comment
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.
Great PR Zedong, thank you for addressing all of my comments. This is ready for review @michaelbynum @jsiirola
|
I think the code coverage has been improved. Can we call CodeCov to generate the coverage report again? |
|
@ZedongPeng, it looks like the coverage of MindtPy has improved, and should not hold up this PR. Reporting is a bit problematic at the moment (the Jenkins builds are currently not running correctly - and are not reporting coverage results, so everything is looking worse than it should in practice). |
jsiirola
left a comment
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.
A couple questions, but nothing to prevent merging (given that the tests are passing). Updated coverage information should be available in ~60 minutes.
| from pyomo.contrib.mcpp import pyomo_mcpp | ||
|
|
||
| required_solvers = ('baron', 'cplex_persistent') | ||
| required_solvers = ('ipopt', 'cplex_persistent') |
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.
Do you not need a global solver for these tests?
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.
I think the Pyomo servers don't have baron installed, so these tests are skipped previously, lowering the code coverage. For these selected models, ipopt is enough to find a global optimal solution.
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.
Baron is present on all testing platforms. On GHA it runs in demo mode (because that is a public testing platform). A licensed version of Baron is available through the Jenkins tests (although Jenkins was down for a couple days, it should be back up now).
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.
Although IPOPT is able to converge in the current version to the optimal solution, I think that this is a fragile test as we have no guarantees that with any newer version of the solvers it will keep working. I would be for reverting this back to baron
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.
OK. @ZedongPeng, if you get the global tests updated this afternoon, we should be able to get this merged in tonight in time for tomorrow's release.
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.
All the problems that we are testing are small enough for the demo license of BARON to work with the NLPs. Let's revent this back to baron but not skip if the license is valid (as the demo version should work)
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.
I just updated the global test. Hope it's not too late.
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.
BARON demo mode handles up to 10 constraints and variables and up to 50 nonlinear operations.
I just added the baron license check, since several models exceed this limitation. @bernalde
Summary/Motivation:
Primal integral, dual integral and primal-dual gap integral are performance measures that reward a balance of speed and solution quality for global and local solvers. This PR adds the calculation of primal integral, dual integral and primal-dual gap in MindtPy.
Changes proposed in this PR:
solve_data.LB,solve_data.UB,solve_data.LB_progressandsolve_data.UB_progressare all replaced bysolve_data.primal_bound,solve_data.dual_bound,solve_data.primal_bound_progressandsolve_data.dual_bound_progress. The log has also been modified.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: