simplify_boolean in the new pattern (9s -> 6s flatten)#959
Conversation
IgnaceBleukx
left a comment
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Primarily doc-related comments
| else: # variables/constants/direct constraints | ||
| changed, newexpr = _simplify_boolean_expr(expr, num_context=num_context) | ||
| if changed: | ||
| assert not isinstance(newexpr, int) |
There was a problem hiding this comment.
You can also assert this by using an overload on the _simplify_boolean_expr header?
If num_context is False, then return type is bool/Expression.
I would expect the num_context on the main transformation to be deprecated? Users should only call this one at the toplevel, where everything is bool
| return False, expr | ||
|
|
||
| elif expr.name == "not": | ||
| if isinstance(args[0], _BoolVarImpl): |
There was a problem hiding this comment.
This case should not happen anymore, right? We should have already pushed down negation. Only case left here should be not(GlobalConstraint)
There was a problem hiding this comment.
Hmm according to the coverage tool, it still fires -- not sure from where
|
This PR merges to |
IgnaceBleukx
left a comment
IgnaceBleukx
left a comment
There was a problem hiding this comment.
Fixed my comments and cleaned up the PR by removing solver-var-1d-related stuff.
Can you also have a look at the final result @tias ?
|
Pumpkin triggers a floating point error (we currently have a test that feeds a 0.5, even though we don't intend to support it). So maybe we should merge #957 first, which removes that case and does foresee the only way we do intend to accept floats in the objective |
2 participants
No description provided.