Repository: sympy/sympy

Issue:
Upgrading to 1.9 broke assumptions with evaluate=False
The following code worked in 1.8:
```
from sympy.core.parameters import evaluate
from sympy.abc import x, h
f=2**x**7
with evaluate(False):
    fh=f.xreplace({x: x+h})
    bool(fh.exp.is_rational)
```

But in 1.9 it raises `TypeError: invalid input: Mod(1*(2*0) + 1*(1*1), (1*1)*(1*2))`

Discussion:
sympy-bot: :white_check_mark:

Hi, I am the [SymPy bot](https://github.com/sympy/sympy-bot) (v167). I'm here to help you write a release notes entry. Please read the [guide on how to write release notes](https://github.com/sympy/sympy/wiki/Writing-Release-Notes).



Your release notes are in good order.

Here is what the release notes will look like:
* core
  * A bug preventing the use of the old assumptions under `evaluate(False)` was fixed. ([#23693](https://github.com/sympy/sympy/pull/23693) by [@oscarbenjamin](https://github.com/oscarbenjamin))

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.11.

<details><summary>Click here to see the pull request description that was parsed.</summary>

    <!-- 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://tinyurl.com/auto-closing for more information). Also, please
    write a comment on that issue linking back to this pull request once it is
    open. -->

    Fixes #22428

    See also the discussion about evaluation in the assumptions system here: https://github.com/sympy/sympy/pull/23488#discussion_r903204398

    #### Brief description of what is fixed or changed

    Avoid creating new expressions during assumptions queries. This makes the old assumptions slightly less powerful but at the same time faster and simpler and more robust. This also fixes a bug making it safe to query the old assumptions under `evaluate(False)`.

    #### Other comments


    #### Release Notes

    <!-- Write the release notes for this release below between the BEGIN and END
    statements. The basic format is a bulleted list with the name of the subpackage
    and the release note for this PR. For example:

    * solvers
      * Added a new solver for logarithmic equations.

    * functions
      * Fixed a bug with log of integers.

    or if no release note(s) should be included use:

    NO ENTRY

    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 -->
    * core
      * A bug preventing the use of the old assumptions under `evaluate(False)` was fixed.
    <!-- END RELEASE NOTES -->

</details><p>

github-actions[bot]: Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with `+` are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with `-` are speedups.

Significantly changed benchmark results (PR vs master)
```diff

```
Significantly changed benchmark results (master vs previous release)
```diff
       before           after         ratio
     [77f1d79c]       [dff24a1f]
     <sympy-1.10.1^0>                 
+         101±1ms        174±0.5ms     1.73  sum.TimeSum.time_doit

```
Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).
oscarbenjamin: The change here exposed a bug in `solveset`. I've improved the checking of solutions to equations with radicals and fixed some tests.
oscarbenjamin: Thanks @smichr. I've incorporated your suggestions with small changes.

I'm marking this as ready for review.
asmeurer: The general idea here about removing recursive evaluation in assumptions is good. 

But can we really rely on assumptions functioning in evaluate=False? Do we know that the basic canonicalization that is disabled by evaluate=False would never be implicitly assumed in an assumption handler? 
oscarbenjamin: > But can we really rely on assumptions functioning in evaluate=False? Do we know that the basic canonicalization that is disabled by evaluate=False would never be implicitly assumed in an assumption handler?

No, we don't know that but really that constraint should be respected by the assumptions system. Code in the assumptions system that does anything else should be changed (and there are many examples).

Patch:
diff --git a/sympy/core/add.py b/sympy/core/add.py
index 651e78cd76..f63869d481 100644
--- a/sympy/core/add.py
+++ b/sympy/core/add.py
@@ -678,8 +678,12 @@ def _eval_is_imaginary(self):
                     return
             elif a.is_imaginary:
                 im_I.append(a*S.ImaginaryUnit)
-            elif (S.ImaginaryUnit*a).is_extended_real:
-                im_I.append(a*S.ImaginaryUnit)
+            elif a.is_Mul and S.ImaginaryUnit in a.args:
+                coeff, ai = a.as_coeff_mul(S.ImaginaryUnit)
+                if ai == (S.ImaginaryUnit,) and coeff.is_extended_real:
+                    im_I.append(-coeff)
+                else:
+                    return
             else:
                 return
         b = self.func(*nz)
@@ -708,8 +712,12 @@ def _eval_is_zero(self):
                     return
             elif a.is_imaginary:
                 im += 1
-            elif (S.ImaginaryUnit*a).is_extended_real:
-                im_or_z = True
+            elif a.is_Mul and S.ImaginaryUnit in a.args:
+                coeff, ai = a.as_coeff_mul(S.ImaginaryUnit)
+                if ai == (S.ImaginaryUnit,) and coeff.is_extended_real:
+                    im_or_z = True
+                else:
+                    return
             else:
                 return
         if z == len(self.args):
diff --git a/sympy/solvers/solveset.py b/sympy/solvers/solveset.py
index 2fd805ddb8..58cfce9eb1 100644
--- a/sympy/solvers/solveset.py
+++ b/sympy/solvers/solveset.py
@@ -879,17 +879,35 @@ def _solve_radical(f, unradf, symbol, solveset_solver):
         result = Union(*[imageset(Lambda(y, g_y), f_y_sols)
                          for g_y in g_y_s])
 
-    if not isinstance(result, FiniteSet):
-        solution_set = result
-    else:
+    def check_finiteset(solutions):
         f_set = []  # solutions for FiniteSet
         c_set = []  # solutions for ConditionSet
-        for s in result:
+        for s in solutions:
             if checksol(f, symbol, s):
                 f_set.append(s)
             else:
                 c_set.append(s)
-        solution_set = FiniteSet(*f_set) + ConditionSet(symbol, Eq(f, 0), FiniteSet(*c_set))
+        return FiniteSet(*f_set) + ConditionSet(symbol, Eq(f, 0), FiniteSet(*c_set))
+
+    def check_set(solutions):
+        if solutions is S.EmptySet:
+            return solutions
+        elif isinstance(solutions, ConditionSet):
+            # XXX: Maybe the base set should be checked?
+            return solutions
+        elif isinstance(solutions, FiniteSet):
+            return check_finiteset(solutions)
+        elif isinstance(solutions, Complement):
+            A, B = solutions.args
+            return Complement(check_set(A), B)
+        elif isinstance(solutions, Union):
+            return Union(*[check_set(s) for s in solutions.args])
+        else:
+            # XXX: There should be more cases checked here. The cases above
+            # are all those that come up in the test suite for now.
+            return solutions
+
+    solution_set = check_set(result)
 
     return solution_set
 


Test patch:
diff --git a/sympy/core/tests/test_assumptions.py b/sympy/core/tests/test_assumptions.py
index fb9839a757..574e90178f 100644
--- a/sympy/core/tests/test_assumptions.py
+++ b/sympy/core/tests/test_assumptions.py
@@ -703,6 +703,18 @@ def test_other_symbol():
     assert x.is_integer is False
 
 
+def test_evaluate_false():
+    # Previously this failed because the assumptions query would make new
+    # expressions and some of the evaluation logic would fail under
+    # evaluate(False).
+    from sympy.core.parameters import evaluate
+    from sympy.abc import x, h
+    f = 2**x**7
+    with evaluate(False):
+        fh = f.xreplace({x: x+h})
+        assert fh.exp.is_rational is None
+
+
 def test_issue_3825():
     """catch: hash instability"""
     x = Symbol("x")
@@ -1158,9 +1170,11 @@ def test_issue_10302():
     r = Symbol('r', real=True)
     u = -(3*2**pi)**(1/pi) + 2*3**(1/pi)
     i = u + u*I
+
     assert i.is_real is None  # w/o simplification this should fail
     assert (u + i).is_zero is None
     assert (1 + i).is_zero is False
+
     a = Dummy('a', zero=True)
     assert (a + I).is_zero is False
     assert (a + r*I).is_zero is None
diff --git a/sympy/solvers/tests/test_solveset.py b/sympy/solvers/tests/test_solveset.py
index a7cb7221df..a1f756a4aa 100644
--- a/sympy/solvers/tests/test_solveset.py
+++ b/sympy/solvers/tests/test_solveset.py
@@ -533,11 +533,9 @@ def test_solve_sqrt_3():
                sqrt(10)*sin(atan(3*sqrt(111)/251)/3)/3 +
                sqrt(30)*cos(atan(3*sqrt(111)/251)/3)/3)]
 
-    assert sol._args[0] == FiniteSet(*fset)
-    assert sol._args[1] == ConditionSet(
-        R,
-        Eq(sqrt(2)*R*sqrt(1/(R + 1)) + (R + 1)*(sqrt(2)*sqrt(1/(R + 1)) - 1), 0),
-        FiniteSet(*cset))
+    fs = FiniteSet(*fset)
+    cs = ConditionSet(R, Eq(eq, 0), FiniteSet(*cset))
+    assert sol == (fs - {-1}) | (cs - {-1})
 
     # the number of real roots will depend on the value of m: for m=1 there are 4
     # and for m=-1 there are none.
@@ -2404,14 +2402,30 @@ def test_issue_11174():
 
 
 def test_issue_11534():
-    # eq and eq2 should give the same solution as a Complement
+    # eq1 and eq2 should not have the same solutions because squaring both
+    # sides of the radical equation introduces a spurious solution branch.
+    # The equations have a symbolic parameter y and it is easy to see that for
+    # y != 0 the solution s1 will not be valid for eq1.
     x = Symbol('x', real=True)
     y = Symbol('y', real=True)
-    eq = -y + x/sqrt(-x**2 + 1)
+    eq1 = -y + x/sqrt(-x**2 + 1)
     eq2 = -y**2 + x**2/(-x**2 + 1)
-    soln = Complement(FiniteSet(-y/sqrt(y**2 + 1), y/sqrt(y**2 + 1)), FiniteSet(-1, 1))
-    assert solveset(eq, x, S.Reals) == soln
-    assert solveset(eq2, x, S.Reals) == soln
+
+    # We get a ConditionSet here because s1 works in eq1 if y is equal to zero
+    # although not for any other value of y. That case is redundant though
+    # because if y=0 then s1=s2 so the solution for eq1 could just be returned
+    # as s2 - {-1, 1}. In fact we have
+    #   |y/sqrt(y**2 + 1)| < 1
+    # So the complements are not needed either. The ideal output here would be
+    #   sol1 = s2
+    #   sol2 = s1 | s2.
+    s1, s2 = FiniteSet(-y/sqrt(y**2 + 1)), FiniteSet(y/sqrt(y**2 + 1))
+    cset = ConditionSet(x, Eq(eq1, 0), s1)
+    sol1 = (s2 - {-1, 1}) | (cset - {-1, 1})
+    sol2 = (s1 | s2) - {-1, 1}
+
+    assert solveset(eq1, x, S.Reals) == sol1
+    assert solveset(eq2, x, S.Reals) == sol2
 
 
 def test_issue_10477():