type-check in operator constructor #297
Conversation
There was a problem hiding this comment.
Looks good, will certainly simplify some edge cases.
Instead of an assert, maybe we should make a "CPMpyTypeError"? Or probably Python has a "TypeError" itself?
To me an "assert" is more a sanity check for the developers that everything works as it should.
Also, a typerror can be catched in Bigtest if any of those models create invalid constraints
| @@ -437,6 +459,10 @@ class Element(GlobalConstraint): | |||
| """ | |||
|
|
|||
| def __init__(self, arr, idx): | |||
| flatargs = flatlist(arr) | |||
| assert not is_boolexpr(idx), "index cannot be a boolexpression: {}".format(idx) | |||
There was a problem hiding this comment.
"boolean expression"?
|
Good to merge! |
|
will add some tests for completeness :) |
Inverse now works when arguments are not ndvararray added tests for the typechecks in global constraints
| @@ -401,6 +401,11 @@ def __init__(self, name, arg_list): | |||
| # sanity checks | |||
| assert (name in Operator.allowed), "Operator {} not allowed".format(name) | |||
| arity, is_bool = Operator.allowed[name] | |||
| if is_bool: | |||
There was a problem hiding this comment.
sorry for being late to review...
you don't need to go into the children if each constructor checks their direct children (can have overhead on deeply nested expressions)
so smth like if is_bool: assert (all(is_boolexpr(a) for a in arg_list), "..."
or not asserts but raises? also fine...
There was a problem hiding this comment.
I'm not sure I understand, just remove the flatlist?
the code is only checking the direct children, the flatlist call just handles cases like wsum were the args consist of 2 list, in stead of one. It can be removed however since there are no such cases for boolean operators
| @@ -172,7 +172,10 @@ class AllDifferent(GlobalConstraint): | |||
| """All arguments have a different (distinct) value | |||
| """ | |||
| def __init__(self, *args): | |||
| super().__init__("alldifferent", flatlist(args)) | |||
| flatargs = flatlist(args) | |||
| if not (all(is_boolexpr(arg) for arg in flatargs) or not any(is_boolexpr(arg) for arg in flatargs)): | |||
There was a problem hiding this comment.
you are looping twice over the args, and on the flat ones even; this surely can trigger (e.g. a boolexpr with an intexpr inside...)
is_boolarg = [is_boolexpr(a) for a in args]; and then on that?
There was a problem hiding this comment.
again, flatlist is not calling flatten, or recursively giving back all children, it just unnests iterables and gives back a list. (since the Alldifferent constructor allows things like AllDifferent([1,2,[1,3]]) and is_ boolexpr on a list will always return false so doing [is_boolexpr(a) for a in args] would run into issues that way
There was a problem hiding this comment.
but certainly it could be more efficient than looping twice, maybe a custom function that checks the type of the first element and then checks if the other elements are of the same type?
| @@ -331,8 +347,8 @@ def value(self): | |||
| # a little helper: | |||
| class IfThenElse(GlobalConstraint): | |||
| def __init__(self, condition, if_true, if_false): | |||
| assert all([is_boolexpr(condition), is_boolexpr(if_true), is_boolexpr(if_false)]), \ | |||
| "only boolean expression allowed in IfThenElse" | |||
| if not all([is_boolexpr(condition), is_boolexpr(if_true), is_boolexpr(if_false)]): | |||
There was a problem hiding this comment.
we don't do that inside if's, its an additional function call..
should be if not is_boolexpr(cond) and is_bool...
or even better if not is_boolex(cond) or not is_boolexp(if_true)...
No description provided.