Linearize multiplication with Boolean and Integer #769
Conversation
|
Some tests are still failing, converting to draft for now |
|
The fix turned out to be proper implementation of Big-M-style reification. As a bonus, this should now also allow to add "truely" linear solvers like Highs, which do not have support for reification in their API |
| else: | ||
| elif "->" in supported and not reified: | ||
| indicator_constraints.append(cond.implies(lin)) # Add indicator constraint | ||
| else: # Nested -> or implication constraints are not supported |
There was a problem hiding this comment.
very minor thing, but this comment threw me off (I thought it'd say something about the case we're in). I'd prefer if the comments was moved to the block that checks this support, or if the asserts themselves just tell us this in their message.
There was a problem hiding this comment.
on the same thread:
- so the assert below should have a message why we expect this comparison, just like the on further below.
- shouldn't we raise the specific excpetion about this, like e.g. TransfNotImplemented ?
There was a problem hiding this comment.
I'll update the comment to make it a bit more clear indeed.
Regarding the asserts: to me this is just a sanity check, and it should always go through if the rest of our code is correct. I.e., the user normally can put in any expression, without triggering this assert
| # bv * iv <comp> rhs, rewrite to (bv -> iv <comp> rhs) & (~bv -> 0 <comp> rhs) | ||
| bv, iv = lhs.args[bv_idx], lhs.args[1-bv_idx] | ||
| bv_true = bv.implies(eval_comparison(cpm_expr.name, iv, rhs)) | ||
| bv_false = (~bv).implies(eval_comparison(cpm_expr.name, 0, rhs)) |
There was a problem hiding this comment.
The extra tests in test_trans_linearize use solveAll, so all cases should be tested by that
Implementation of multiplication constraints of the form
bv * iv <comp> rhsThese are now translated to
bv -> iv <comp> rhs & (~bv) -> 0 <comp> rhsThe only solver affected is CPLEX, which does not really support multiplication anyway... Our interface kept
mulin the model previously, but always raised an error when it was about to be posted.Additionally, the Cumulative decomposition relies on Boolean-Integer multiplication, which does not work on the current master.
Related to #764