-
Notifications
You must be signed in to change notification settings - Fork 517
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
GDP => MINLP Transformation #3082
Conversation
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.
@michaelbynum you are reading our minds! We were just discussing about this transformation with @emma58 @jsiirola and @ZedongPeng
The code looks great, but since we have the chance of creating a new reformulation, should we use it to give it a good name? GDP to MINLP is not specific enough (as BM and Hull might do that as well). How about binary multiplication? Short and descriptive!
Great suggestion! I like it! |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3082 +/- ##
==========================================
+ Coverage 88.28% 88.31% +0.03%
==========================================
Files 832 833 +1
Lines 92307 94881 +2574
==========================================
+ Hits 81495 83798 +2303
- Misses 10812 11083 +271
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thanks so much for doing this, @michaelbynum! A few comments (most of which are my mistakes propagating, sorry...), but this generally looks good.
rhs = 1 if parent_disjunct is None else parent_disjunct.binary_indicator_var | ||
if obj.xor: | ||
xorConstraint[index] = or_expr == rhs |
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.
Unfortunately (and this is my fault too), this isn't actually correct... It assumes that nested indicator variables are local, which is not necessarily true. The safest thing for now is to always set the rhs to 1 and to put the exactly-one constraint on the parent block (and transform from leaf to root, which I think you're already doing).
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.
Your point with putting the exactly-one constraint on the parent block (instead of the root disjunct's parent block) is that the exactly-one constraint needs to be transformed again if it is for a nested disjunction?
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.
Maybe I'm misunderstanding what the "root disjunct" is...
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 putting the exactly-one constraint on the parent disjunct equivalent?
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.
Correct. It should be on the parent disjunct so that it does get transformed again.
Thanks for all the feedback, @emma58. |
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.
Two really minor things, but nothing to prevent merging
Summary/Motivation:
This PR adds a GDP transformation to convert a GDP to a MINLP by converting f(x) <= 0 on a disjunct to f(x)*y <= 0.
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: