Save expr == var in csemap to catch silly user mistakes#972
Save expr == var in csemap to catch silly user mistakes#972IgnaceBleukx wants to merge 4 commits into
Conversation
| # save expr == var to the csemap, so we don't make a new variable for expr later | ||
| if csemap is not None and exprname == '==': # cheap checks first | ||
| if isinstance(lexpr, Expression) and isinstance(rexpr, Expression) and \ | ||
| lexpr.is_bool() == rexpr.is_bool() and __is_flat_var(rexpr): # types must match |
There was a problem hiding this comment.
isinstance(rexpr, Expression) and __is_flat_var(rexpr) sounds like isinstance(rexpr, IntVarImpl)?
and what if its var == expr? or is that already normalized out before this line?
| if csemap is not None and exprname == '==': # cheap checks first | ||
| if isinstance(lexpr, Expression) and isinstance(rexpr, Expression) and \ | ||
| lexpr.is_bool() == rexpr.is_bool() and __is_flat_var(rexpr): # types must match | ||
| csemap[lexpr] = rexpr # save lexpr == rexpr to the csemap |
There was a problem hiding this comment.
what if lexpr is already in? we would overwrite it?
if its in, we could post rexpr == csemap[lexpr] and store csemap[rexpr] == csemap[lexpr]?
|
|
||
| def __setitem__(self, attr, val): | ||
| raise ValueError("__setitem__ is not supported for flat_map, use get_or_make_var instead") | ||
| def __setitem__(self, attr: Expression, val: _IntVarImpl) -> None: |
There was a problem hiding this comment.
I agree we need it for the code below, but maybe add a doc "Use get_or_make_var() instead, which will create a fresh variable for you as needed. Only use this function if you must register an existing attr == val.
|
Already implemented for booleans via #1010; too be revisited in another PR if we want to do this for int expressions too. |
|
I did not realize these were 2 separate commits; it makes sense to do it for integers too yes; lets keep it open for a little longer to remind ourselves? |
2 participants
As discussed offline, it would be useful to catch silly mistakes by the user, where an aux var is introduced for some expression, but then the expression is used later on as a nested expression somewhere.
To fix this, this PR saves the
csemap[expr] = varwhenever we encounter a top-level==constraint.