-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
283 use BoolVal in stead of True and False #284
Conversation
time for test suite: vs master |
In retrospect I am not sure how reliable the testsuite is for time-benchmarking. I guess we would need a repo of exectuable python scripts that return a model, not even the pickles we have in bigtest, if we also want to benchmark model creation time... that is definitely for later ; ) Also you would have to compare the difference of just 1 commit to know the effect of that one change. Anyway, on master all pass tests in ~3.5 minutes here, but this branch FAILED tests/test_examples.py::test_examples[./examples/knapsack.py] - TypeError: int() argument must be a string, a bytes-like object or a number, not 'Operator' e.g.
I guess if it passes the tests we should not be too worried by another function call at creation time, at this point. |
ah the examples don't run on my machine and they seem to run but not fail on the automated tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments for change on built-in overwrites.
will merge
return False # no need to create constraint | ||
elif elem is True or elem is np.True_: | ||
if is_false_cst(elem): | ||
return BoolVal(False) # no need to create constraint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can/should in general change the return type of a Python builtin...
'any' can be used a lot in an arbitrary code-base that only includes cpmpy for a small part
maybe if we check that at least one element is a CPMpy expr, but for now I will just undo this
|
||
def max(iterable): | ||
""" | ||
max() overwrites python built-in, | ||
checks if all constants and computes np.max() in that case | ||
""" | ||
if not any(isinstance(elem, Expression) for elem in iterable): | ||
if is_false_cst(any(isinstance(elem, Expression) for elem in iterable)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this already shows that returning a custom type for 'any' is not a good idea...
Note that I now implemented __bool__()
for BoolVal, so the original code would work now (with not), but still, better without for now
@@ -97,6 +98,6 @@ def sum(iterable): | |||
otherwise, makes a sum Operator directly on `iterable` | |||
""" | |||
iterable = list(iterable) # Fix generator polling | |||
if not any(isinstance(elem, Expression) for elem in iterable): | |||
if is_false_cst(any(isinstance(elem, Expression) for elem in iterable)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what is breaking the tests on my machine
(does this mean that the examples are not run on the github CI? nor on your computer?)
Had to do a little bit more work than just replacing the return true and false so made it into a pr