Not operator #225
Not operator #225
Conversation
|
Branches from someglobals |
|
It is hard to review when the diff also contains other MRs. Thinking about how to resolve this... Maybe: |
# Conflicts: # tests/test_globalconstraints.py
not_xor test
There was a problem hiding this comment.
also in line 517 of core.py, there is something weird...
we check that if 'x == 1' and x is_bool, then it is just 'x'. And because we overwrite 'eq' for this, we also need to overwrite hash, which is a bit ugly.
But we (I) implemented it rather inconsistently... we do not check for != 0, or that !=1 on bool would be == 0... or (after this commit) that ==0 should be ~x. I wonder though that may be we should leave it the way the user wrote it here, and leave it for 'normalize' to normalize these? (removing can be here, normalizing would be in a separate commit ; )
|
Will the new transformation |
# Conflicts: # cpmpy/expressions/__init__.py # cpmpy/expressions/globalconstraints.py # tests/test_globalconstraints.py
Is overwriting hash a big drawback? Otherwise it could be great to get rid of all these cases in our code where we have to take into account that we can compare booleans with 0. But maybe it is a futile effort anyway because we still allow boolexpr == numexpr? |
|
Getting rid of all cases is through a normalize transformation, e.g. to also capture boolvar() >= 1 etc. Our design principle is that expressions as posted should be as close as possible to what the user wrote (e.g. when they print it immediately after). So we can remove trivial things like a + 0 :: a, and I guess b == 1 to b, but nothing too clever/surprising. |
|
Ok so i think we will leave it to a separate transformation to do these cleanups, also the push down negation will be in another pr |
| if hasattr(expr, "decompose_negation"): | ||
| # for global constraints where the negation of the decomposition is not equivalent | ||
| # to the negated global constraint (due to auxiliary variables, i.e. Circuit) | ||
| return Operator('and', expr.decompose_negation()) |
There was a problem hiding this comment.
we are not very consistent with the use of our builtin-overwritten any() and Operator('and',...).
one benefit of all() is that it removes constants from the args, one benefit of Operator is that we dont have to import our all, and that we save the effort of a function call and going over the loop...
we were already not consistent, so not for you to fix in this PR, but to keep in mind that we might want to make it consistent at some point...
No description provided.