-
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
Adding logical_to_disjunctive, moving GDP transformations from logical-to-linear to logical-to-disjunctive #2627
Conversation
…ments to atmost atleast and exactly
…if it works yet because tests need rewriting too
… assertExpressionsStructurallyEqual (so that the same number as an int and as a float is equal)
…DP transformations because logical_to_disjunctive doesn't create them.
…isjunctive, but making an option for another transformation if desired
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.
This looks good! One minor performance-related comment, but doesn't need to hold up this PR
(missed that there were significant failing tests in Jenkins)
Marking this as WIP for now--I didn't realize there are tests in core that call bigm and hull, so I need to fix those. |
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 this looks great. One question: I do agree that we should switch GDP from logical_to_linear
to using logical_to_disjunctive
; however, do we want/need to deprecate logical_to_linear
? While the implementation has several limitations, it does provide a somewhat different capability (the CNF form).
Codecov ReportBase: 87.01% // Head: 87.05% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #2627 +/- ##
==========================================
+ Coverage 87.01% 87.05% +0.04%
==========================================
Files 754 757 +3
Lines 84078 84360 +282
==========================================
+ Hits 73160 73441 +281
- Misses 10918 10919 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
@jsiirola, okay, I un-deprecated logical to linear. I think we should rename it logical_to_cnf, but we can do that in a separate PR, |
Fixes # .
Summary/Motivation:
The
core.logical_to_linear
transformation is very slow because it uses sympy to convert logical expressions to CNF. Since we don't really need them in CNF in order to write a MIP (which is the goal of the transformation), this PR adds a transformation tocontrib.cp
calledlogical_to_disjunctive
that transforms logical constraints into disjunctive programs in the sense that 1) logical constraints on only Boolean variables are converted to algebraic constraints on binary variables, but 2) logical constraints on mixtures of Boolean and integer variables (such asexactly
,atmost
, andatleast
) are transformed into disjunctions over binaries and integers. (Note to future us: We're using the term "disjunctive program" rather than GDP intentionally here, as the promise of the transformation is to no longer have any logical expressions in the model: only algebraic constraints and disjunctions of algebraic constraints.)This reduces the
gdp.bigm
transformation time for a certain model that builds in 15 seconds from 129 seconds (current main branch) to 75 seconds (this branch).NOTE: This is currently failing tests because of #2626 (because there are logical constraints in the pickle tests andlogical_to_disjunctive
fixes some variables.)Changes proposed in this PR:
contrib.logical_to_disjunctive
transformation tocontrib.cp
Deprecatescore.logical_to_linear
, mainly because it will not be easily extensible as we add to the set of supported nodes in the logical expression system. For example,exactly
,atmost
, andatleast
are handled with ad hoc bigm-ing right now--we would have to keep adding special handling as we includealldiff
, etc.contrib.logical_to_disjunctive
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: