-
Notifications
You must be signed in to change notification settings - Fork 64
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
Evaluate Relationals when casting to bool #354
base: master
Are you sure you want to change the base?
Evaluate Relationals when casting to bool #354
Conversation
…ean evaluation is, instead of always True. Resolves symengine#312
This currently fails
That final assertion fails, but the statement is not mathematically true. Subtracting
|
Fixed incorrect evaluation test. To get rid of a sin(x)**2, we need to subtract sin(x)**2, not x**2.
In fact, I feel that some of those tests should have some "not equal" test counterparts. The test was only passing before because of the bug in Issue #312. This issue would have been caught with the presence of such a test. |
I don't like the inexact calculations. If symengine doesn't know that the equality is True or False exactly, then it shouldn't do the simplification. |
In retrospect, I agree. I will get rid of approximations in a commit tomorrow. Do you feel it would be appropriate in the event that symengine cannot exactly do such a calculation, to still outsource it sympy (if available)? |
… a ValueError if simplification is unclear.
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 should be done using symengine simplify
, doit
or similar directly. We should avoid having to much code in the language bindings. That being said I still think this can be merged in the meantime.
# If we still cannot determine whether or not the relational is true, then we can either outsource the | ||
# evaluation to sympy (if available) or raise a ValueError expressing that the evaluation is unclear. | ||
try: | ||
return bool(self.simplify()) |
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 will return True
for 2*(x + 1) - 2
which is different from what we get for 2*x
which is a TypeError.
References to other Issues or PRs
Fixes #312. Before, a Relational operator would always evaluate to True when cast as a bool, regardless of whether or not its left and right hand sides were actually equal. This fixes that. Additionally, if the evaluation is ambiguous (namely, there are free variables), it throws a TypeError.
Brief description of what is fixed or changed
This modifies the class definition of Relational in the symengine python wrapper by overriding bool. If the relation can sucessfully and unambiguously evaluated as a bool (i.e. both sides are constants with no free variables), then it does the simplification using its simplify function.
If the relational's boolean value is ambiguous (i.e. it has free symbols), then it throws the
TypeError(f'Relational with free symbols cannot be cast as bool: {self}')
.This is closer to the expected behavior of the bool function on these types.
Other comments
The TypeError check will subtract one side from the other, and then expand out the expression. If that expansion has no free symbols, then the expression will evaluate normally.