-
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
cover cp.abs() using the builtin #513
Conversation
Maybe just put a comment on why specifically we import it here? |
this is too sneaky for me, it will be a hidden feature not present in our docs or visible to standard users. The right way is a new function in python_builtins, with a one-liner documentation, that calls the Will also make it easier to (generate) tests for... |
Random not related test is failing .... Will dig into it |
Looks good, thanks for adding tests as well, can be merged |
remove redundant 'abs' code
Do we still want to allow using the builtin abs on our expressions, or should we enforce cpmpy.abs? I guess it's fine to allow it |
@tias looks like you have to approve this now that the 'not' is added :) |
bit more explicit...
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.
changed to make the global in the if (without not), seems more logical
also added a comment on the 'any_list', which I still find a bit weird but not a blocker : ) e.g. why it has to be a cpm_array... could have done map(abs, arg) or so, but maybe not worth any further thought
# Conflicts: # tests/test_globalconstraints.py
To cover the case of cp.abs().
Currently, we do not override abs(), so it fails, although if we do from cpmpy import * just using directly abs() works (because we override it directly in the variables).
just including from builtins import abs in init.py of expressions tackles it