Repository: sympy/sympy
Pull request: allow And/Or.subs to short-circuit
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->

closes #17530

#### Brief description of what is fixed or changed

When And or Or can evaluate during substitution they are allowed to do so and any args that would generate an error are ignored unless the substitution doesn't short-circuit -- then the error is raised.

#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* logic
    * And/Or will supress errors if evaluation of other args during subs gives False/True
<!-- END RELEASE NOTES -->


Problem statement:
should And and Or short-circuit and avoid invalid args if possible
For ` Or(x+y>0, x - y < 0).subs(x,oo).subs(y,oo)` return True or raise an Error? While `x + y` is defined and it is positive, the difference evaluates to nan and the comparison raises a TypeError. If short-circuiting is better than raising an error then this diff might be of use:

```diff
diff --git a/sympy/logic/boolalg.py b/sympy/logic/boolalg.py
index bc1da80..49b51c6 100644
--- a/sympy/logic/boolalg.py
+++ b/sympy/logic/boolalg.py
@@ -40,6 +40,8 @@ def as_Boolean(e):

     """
     from sympy.core.symbol import Symbol
+    if e is S.NaN:
+        return e
     if e == True:
         return S.true
     if e == False:
@@ -679,6 +681,25 @@ def _new_args_filter(cls, args):
             newargs.append(x)
         return LatticeOp._new_args_filter(newargs, And)

+    def _eval_subs(self, old, new):
+        args = []
+        bad = None
+        for i in self.args:
+            try:
+                i = i.subs(old, new)
+            except:  # store any problem
+                if bad is None:
+                    bad = i
+                continue
+            if i == False:
+                return S.false
+            elif i != True:
+                args.append(i)
+        if bad is not None:
+            # let it raise
+            bad.subs(old, new)
+        return self.func(*args)
+
     def _eval_simplify(self, **kwargs):
         from sympy.core.relational import Equality, Relational
         from sympy.solvers.solveset import linear_coeffs
@@ -797,6 +818,25 @@ def _new_args_filter(cls, args):
             newargs.append(x)
         return LatticeOp._new_args_filter(newargs, Or)

+    def _eval_subs(self, old, new):
+        args = []
+        bad = None
+        for i in self.args:
+            try:
+                i = i.subs(old, new)
+            except:  # store any problem
+                if bad is None:
+                    bad = i
+                continue
+            if i == True:
+                return S.true
+            elif i != False:
+                args.append(i)
+        if bad is not None:
+            # let it raise
+            bad.subs(old, new)
+        return self.func(*args)
+
     def _eval_as_set(self):
         from sympy.sets.sets import Union
         return Union(*[arg.as_set() for arg in self.args])
```

Hints from issue discussion:
Not sure about the answer to your question but this is potentially a reason for using `x < y` rather than `x - y < 0` since:
```
In [4]: Or(x+y>0, x < y).subs(x,oo).subs(y, oo)                                                                                   
Out[4]: True
```
I don't know if True makes sense in the original context of your problem though...
It is a toy problem to demonstrate the issue. The nan could arise in a variety of ways. I'm not sure that one should have to worry about how to write an expression if other args of the Or are meant to check for invalid conditions but don't evaluate until substitution. (This is coming up in work related to #17146.)
I think short-circuiting is better because if something else invalid hasn't come into picture then why to raise error for it.

Commit message: 17530: allow And/Or.subs to short-circuit

Patch to review:
diff --git a/sympy/logic/boolalg.py b/sympy/logic/boolalg.py
index bc1da809e4ce..72c83c4f830c 100644
--- a/sympy/logic/boolalg.py
+++ b/sympy/logic/boolalg.py
@@ -679,6 +679,25 @@ def _new_args_filter(cls, args):
             newargs.append(x)
         return LatticeOp._new_args_filter(newargs, And)
 
+    def _eval_subs(self, old, new):
+        args = []
+        bad = None
+        for i in self.args:
+            try:
+                i = i.subs(old, new)
+            except:  # store any problem
+                if bad is None:
+                    bad = i
+                continue
+            if i == False:
+                return S.false
+            elif i != True:
+                args.append(i)
+        if bad is not None:
+            # let it raise
+            bad.subs(old, new)
+        return self.func(*args)
+
     def _eval_simplify(self, **kwargs):
         from sympy.core.relational import Equality, Relational
         from sympy.solvers.solveset import linear_coeffs
@@ -797,6 +816,25 @@ def _new_args_filter(cls, args):
             newargs.append(x)
         return LatticeOp._new_args_filter(newargs, Or)
 
+    def _eval_subs(self, old, new):
+        args = []
+        bad = None
+        for i in self.args:
+            try:
+                i = i.subs(old, new)
+            except:  # store any problem
+                if bad is None:
+                    bad = i
+                continue
+            if i == True:
+                return S.true
+            elif i != False:
+                args.append(i)
+        if bad is not None:
+            # let it raise
+            bad.subs(old, new)
+        return self.func(*args)
+
     def _eval_as_set(self):
         from sympy.sets.sets import Union
         return Union(*[arg.as_set() for arg in self.args])
diff --git a/sympy/logic/tests/test_boolalg.py b/sympy/logic/tests/test_boolalg.py
index 8ced2bfaec27..ac91baccc1f8 100644
--- a/sympy/logic/tests/test_boolalg.py
+++ b/sympy/logic/tests/test_boolalg.py
@@ -1079,3 +1079,12 @@ def test_issue_16803():
     # No simplification done, but should not raise an exception
     assert ((n > 3) | (n < 0) | ((n > 0) & (n < 3))).simplify() == \
         ((n > 3) | (n < 0) | ((n > 0) & (n < 3)))
+
+
+def test_issue_17530():
+    r = {x: oo, y: oo}
+    assert Or(x + y > 0, x - y < 0).subs(r)
+    assert not And(x + y < 0, x - y < 0).subs(r)
+    raises(TypeError, lambda: Or(x + y < 0, x - y < 0).subs(r))
+    raises(TypeError, lambda: And(x + y > 0, x - y < 0).subs(r))
+    raises(TypeError, lambda: And(x + y > 0, x - y < 0).subs(r))


Code review comments:
diff_hunk: @@ -679,6 +679,25 @@ def _new_args_filter(cls, args):
             newargs.append(x)
         return LatticeOp._new_args_filter(newargs, And)
 
+    def _eval_subs(self, old, new):
+        args = []
+        bad = None
+        for i in self.args:
+            try:
+                i = i.subs(old, new)
+            except:  # store any problem
path: sympy/logic/boolalg.py
text: @user1:
I would read that as "hide any problem" :)

Bare except shouldn't be used anywhere in SymPy. Catching Exception is preferable to bare except (since it won't catch e.g. KeyboardInterrupt) but is still bad.

What exception is being caught here? I guess it's TypeError (invalid nan comparison). Probably we should change it so that we have an InvalidComparisonError to be caught in these situations though.

@user1:
#17537 

@author:
> Bare except shouldn't be used anywhere in SymPy

import this :-)

In this case the error is only being deferred and if there is no short circuit then the error that was first suppressed will be raised below.

@user1:
Ah I see. It's still better to catch Exception rather than use bare except for that though.

In the case where this does short-circuit the exception will be suppressed and we need to be more specific about which exceptions to suppress if we're doing that. I would suggest catching only the exceptions you expect to see: anything else could be a bug somewhere.

@author:
OK...easy enough to add any other obscure errors that arise.