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

Various bug fixes in gdp.mbigm transformation #3073

Merged
merged 23 commits into from
Feb 19, 2024
Merged

Conversation

emma58
Copy link
Contributor

@emma58 emma58 commented Dec 21, 2023

Fixes # .

Summary/Motivation:

Fixes three bugs in the multiple bigm transformation:

  1. There was an indentation error that was revealed by having (active) empty constraint containers on Disjuncts. Now we only deactivate constraints that exist, wheee.
  2. Not all Suffixes are bad Suffixes: Ceasing to complain when we ignore Suffixes, but adding a note to the documentation that we will ignore BigM Suffixes, since that might not be intuitive. (We're also safe that this will never change because the keys will always be different for bigm and mbigm: mbigm needs to know the Disjunct the M is relative to as well.)
  3. Disjuncts can be infeasible--it's okay: When the solve to calculate M comes back infeasible, this means the Disjunct is. We log that fact at the DEBUG level, and properly deactivate the Disjunct. I'm setting the M values to 0 for now for the sake of being explicit.

Changes proposed in this PR:

  • Fixes to the above
  • Tests!

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.

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

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

Comparison is base (f81f34f) 88.34% compared to head (affa10c) 88.33%.
Report is 13 commits behind head on main.

Files Patch % Lines
pyomo/gdp/plugins/multiple_bigm.py 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3073      +/-   ##
==========================================
- Coverage   88.34%   88.33%   -0.01%     
==========================================
  Files         833      833              
  Lines       92567    92582      +15     
==========================================
+ Hits        81776    81786      +10     
- Misses      10791    10796       +5     
Flag Coverage Δ
linux 86.23% <96.29%> (+<0.01%) ⬆️
osx 75.69% <92.59%> (+<0.01%) ⬆️
other 86.43% <96.29%> (-0.01%) ⬇️
win 83.64% <96.29%> (-0.01%) ⬇️

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.

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

A couple general questions

Comment on lines 633 to 636
if (
results.solver.termination_condition
is TerminationCondition.infeasible
):
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that we want to check against "infeasible"? For the "old" (current) solver interfaces, I am not sure if this is always a guarantee of infeasibility. (In particular, I don't think that Ipopt will ever guarantee infeasibility)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this so that we maintain a list of solvers we trust that 'infeasible' means 'proven infeasible' and we do this for those. For others (looking at you, ipopt), we just revert to the old behavior and scream about an unsuccessful solve.

Comment on lines 664 to 667
if (
results.solver.termination_condition
is TerminationCondition.infeasible
):
Copy link
Member

Choose a reason for hiding this comment

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

Same question on infeasible here...

@@ -201,7 +200,6 @@ class MultipleBigMTransformation(GDP_to_MIP_Transformation, _BigM_MixIn):

def __init__(self):
super().__init__(logger)
self.handlers[Suffix] = self._warn_for_active_suffix
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we want to silently ignore Suffixes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to ignore LocalVars, warn about ignoring BigM, and yell for everything else.

@emma58
Copy link
Contributor Author

emma58 commented Feb 15, 2024

OK, @jsiirola, this is ready for another look.

self, other_disjunct, scratch_block, unsuccessful_solve_msg
):
solver = self._config.solver
solver_trusted = solver.name in _trusted_solvers
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can rely on solver.name here:

>>> SolverFactory('gurobi_direct').name
'gurobi_direct'
>>> SolverFactory('gurobi_persistent').name
'gurobi_persistent'
>>> SolverFactory('gurobi', solver_io='lp').name
'gurobi'
>>> SolverFactory('gurobi', solver_io='nl').name
'asl'
>>> SolverFactory('appsi_gurobi').name
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: 'LegacySolver' object has no attribute 'name'

If we are going to do this, maybe the solution is that the solvers advertise two new attributes: .algorithm and .interface? @mrmundt, @michaelbynum?

emma58 and others added 2 commits February 18, 2024 14:07
…ted solver name for now, and adding a TODO about how the future will be better.
@mrmundt mrmundt merged commit ddaa492 into Pyomo:main Feb 19, 2024
33 checks passed
@emma58 emma58 deleted the mbigm-fixes branch February 21, 2024 16:39
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.

4 participants