symengine.py icon indicating copy to clipboard operation
symengine.py copied to clipboard

Evaluate Relationals when casting to bool

Open qthequartermasterman opened this issue 3 years ago • 4 comments

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.

qthequartermasterman avatar Jun 05 '21 14:06 qthequartermasterman

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

qthequartermasterman avatar Jun 05 '21 20:06 qthequartermasterman

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.

qthequartermasterman avatar Jun 05 '21 21:06 qthequartermasterman

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.

isuruf avatar Jun 08 '21 03:06 isuruf

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

qthequartermasterman avatar Jun 08 '21 04:06 qthequartermasterman