Skip to content
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

Fix MindtPy bugs #3034

Merged
merged 73 commits into from
Feb 14, 2024
Merged

Fix MindtPy bugs #3034

merged 73 commits into from
Feb 14, 2024

Conversation

ZedongPeng
Copy link
Contributor

Fixes # .

  1. fix gurobi single tree termination check bug
  2. fix Gurobi single tree cycle handling
  3. improve copy_var_list_values function
  4. fix bug in feasibility pump method
  5. add special handle for infeasible relaxed NLP
  6. update the log format of infeasible fixed NLP subproblems
  7. create new copy_var_list_values function

Summary/Motivation:

This PR fixes several bugs in MindtPy according to the benchmark results.

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:

  1. I agree my contributions are submitted under the BSD license.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

@blnicho
Copy link
Member

blnicho commented Nov 21, 2023

This is waiting for #2988 to be merged first

Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments and questions. I also think it would be good to add tests for the bugs you are addressing in this PR.

pyomo/contrib/mindtpy/algorithm_base_class.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/algorithm_base_class.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/algorithm_base_class.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/util.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/util.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (d5aca8b) 88.29% compared to head (66f5025) 88.32%.
Report is 9 commits behind head on main.

Files Patch % Lines
pyomo/contrib/mindtpy/algorithm_base_class.py 65.51% 10 Missing ⚠️
pyomo/contrib/mindtpy/util.py 88.63% 5 Missing ⚠️
pyomo/contrib/mindtpy/cut_generation.py 0.00% 3 Missing ⚠️
pyomo/contrib/mindtpy/extended_cutting_plane.py 33.33% 2 Missing ⚠️
...yomo/contrib/mindtpy/global_outer_approximation.py 0.00% 2 Missing ⚠️
pyomo/contrib/mindtpy/single_tree.py 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3034      +/-   ##
==========================================
+ Coverage   88.29%   88.32%   +0.03%     
==========================================
  Files         832      832              
  Lines       92234    92253      +19     
==========================================
+ Hits        81438    81484      +46     
+ Misses      10796    10769      -27     
Flag Coverage Δ
linux 86.20% <75.51%> (+0.15%) ⬆️
osx 75.63% <24.48%> (+0.07%) ⬆️
other 86.38% <75.51%> (+0.15%) ⬆️
win 83.62% <75.51%> (+0.16%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@emma58 emma58 self-requested a review January 23, 2024 20:02
@ZedongPeng
Copy link
Contributor Author

The osx/3.9 test encountered a failure, which appears to be due to an internet connectivity issue. The specific error message was: Error: nodename nor servname provided, or not known (api.github.com:443).
Based on the overall progress, I believe this PR is now ready for merging. Could you please give it a final review, @emma58?

Copy link
Contributor

@emma58 emma58 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions, but overall this looks good. Thanks for the added tests!

@@ -1 +1 @@
__version__ = (0, 1, 0)
__version__ = (1, 0, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should your changelog in line 54 of Mindtpy.py match this version number?

pyomo/contrib/mindtpy/algorithm_base_class.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/algorithm_base_class.py Outdated Show resolved Hide resolved
Comment on lines 110 to +112
except KeyError as e:
self.config.logger.error(str(e) + '\nDeactivating no-good cuts failed.')
self.config.logger.error(e, exc_info=True)
self.config.logger.error('Deactivating no-good cuts failed.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can this fail? Wouldn't this be a DeveloperError (if you need an error here at all) since mindtpy can keep track of what cuts it has added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I checked the log of the benchmark tests and found this error came out when timelimit was reached. Let me try to fix it before merging this PR. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The KeyError happens when no feasible solution has been found within the timelimit. I added one more condition for the fix_dual_bound function in the OA, GOA, LP/NLP B&B and GLP/NLP B&B methods.
I also removed fix_dual_bound for the ECP method since we will never add no-good cuts in the ECP method.
I would appreciate it if @bernalde can give it a double-check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. I think a different error message might be clearer then--it's not deactivating the cuts that failed, it's the solve after you deactivated them (if I'm understanding correctly).

@ZedongPeng
Copy link
Contributor Author

I have already run the black format locally. Are there any new requirements for the black format?

~/Github/pyomo/pyomo/contrib/mindtpy benchmark *1 !2 ❯ black -S -C ./
All done! ✨ 🍰 ✨
40 files left unchanged.

@mrmundt
Copy link
Contributor

mrmundt commented Feb 1, 2024

@ZedongPeng - check your black version. They released an update this month that made some changes.

Copy link
Member

@blnicho blnicho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZedongPeng I have a couple minor edits but otherwise I think this looks good.

pyomo/contrib/mindtpy/util.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/util.py Outdated Show resolved Hide resolved
pyomo/contrib/mindtpy/util.py Outdated Show resolved Hide resolved
ZedongPeng and others added 3 commits February 13, 2024 13:24
Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
Co-authored-by: Bethany Nicholson <blnicho@users.noreply.github.com>
@blnicho blnicho merged commit 5903fbc into Pyomo:main Feb 14, 2024
28 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants