Conversation
IgnaceBleukx
left a comment
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Great, that should do away with a lot of weird cases in the transformation stack...
Maybe also add the optional argument to expressions.utils.implies? We use it when we're not sure the lhs is an expression
IgnaceBleukx
left a comment
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Minor comments on docs, and one potential simplification in implies that we can always do, regardless of the simplify arg
| Expression: the implication constraint or a BoolVal if simplified | ||
|
|
||
| Simplification rules: | ||
| - BoolVal(True) -> other :: other (BoolVal-ified if needed) |
There was a problem hiding this comment.
We can always apply this one, it does not remove user vars
There was a problem hiding this comment.
yes... but then the doc needs to be updated,
you are suggesting that we do:
- if self is true, we post other
- otherwise, if simplify then we remove other else we post the reification as is
while now, we do 'simplify' -> simplify it; 'no simplify' -> post as is
There was a problem hiding this comment.
Yes, I would be in favor of updating the docs and always doing the simplification.
It is in line with what we do for sum/prod etc, when there is a neutral element.
E.g., x + 0 remains x, instead of writing a sum.
It also ties to #342
There was a problem hiding this comment.
Done.
Getting the typing right makes me realize/fear we should introduce a stricter
BoolExprLike: TypeAlias = Union["Expression", bool, np.bool_] # subtype of ExprLike (bool subtype int)
which should be a separate PR as it will affect all logical constraints/operators/functions?
There was a problem hiding this comment.
Why would we need that? I would expect methods on expressions to always return an expression? Even if it is a BoolVal, it should be a CPMpy Expression object and not a Python/Numpy bool
There was a problem hiding this comment.
for user-facing functions like implies: can take a BoolExpr, can not take an int...
There was a problem hiding this comment.
(so for the argument, not the return type idd)
There was a problem hiding this comment.
Aha yes I see; indeed...
tias
left a comment
tias
left a comment
There was a problem hiding this comment.
see 2 remaining comments
2 participants
one notable change is about 'implies' and simplification...
one principle is that we typically don't simplify expressions at creation time, only at transformation time (subject to some exceptions).
I now make this more explicit, making 'simplify' an option to implies (which we don't activate anywhere at the moment). So for the user we wont delete expressions and variables unexpectedly, and for us, if we want it to activate we can.