Skip to content
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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

qthequartermasterman
Copy link

@qthequartermasterman qthequartermasterman commented Jun 5, 2021

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.

…ean evaluation is, instead of always True. Resolves symengine#312
@qthequartermasterman
Copy link
Author

This currently fails test_eval_double2() in test_eval.py. I feel that this test is incorrect. The test is currently written as this:

def test_eval_double2():
    x = Symbol("x")
    e = sin(x)**2 + sqrt(2)
    raises(RuntimeError, lambda: e.n(real=True))
    assert abs(e.n() - x**2 - 1.414) < 1e-3

That final assertion fails, but the statement is not mathematically true. Subtracting x**2 from sin(x)**2 does not cancel out the original sin function. This difference can be arbitrarily big. I believe it should be written (and was intended to be) as such:

def test_eval_double2():
    x = Symbol("x")
    e = sin(x)**2 + sqrt(2)
    raises(RuntimeError, lambda: e.n(real=True))
    assert abs(e.n() - sin(x)**2.0 - 1.414) < 1e-3

Fixed incorrect evaluation test. To get rid of a sin(x)**2, we need to subtract sin(x)**2, not x**2.
@qthequartermasterman
Copy link
Author

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.

@isuruf
Copy link
Member

isuruf commented Jun 8, 2021

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.

@qthequartermasterman
Copy link
Author

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)?

Copy link
Contributor

@rikardn rikardn left a 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())
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relational operators always evaluate to True when cast to bool
3 participants