Repository: sympy/sympy
Pull request: Fixed behaviour of S.Complexes in unions (#11730)
This PR fixes #11730 

Problem statement:
Union(FiniteSet(oo), S.Complexes) gives S.Complexes (should remain unevaluated)
Hi, well searching i found this:

``` python
>>> oo in S.UniversalSet
True
>>> oo in S.Complexes
False
>>> Union(FiniteSet(oo), S.Complexes)
S.Complexes
```

i don't know with this where `oo` belongs, is part of Complexes or not?

Thx. Cya.

Hints from issue discussion:
UniversalSet doesn't come into play here. It's just a formal set that always returns True for any containment check. `Union(FiniteSet(oo), S.Complexes)` giving `S.Complexes` is a bug. 

(Optimistically setting this as easy to fix. I suspect it isn't difficult, but there is a chance I am wrong)

After working for a bit on this, there's also a related bug with Intersection

``` python
>>> Intersection(S.Reals, ComplexRegion(Interval(0,1)**2))
ℝ
```

One would expect this to be [0,1].

I have tried fixing this [here](https://github.com/MrOverlord/sympy/commit/efa9a618bb277612c1520eaea01264a866d654b0#diff-c1c17bdd28aafd400bf9415c7d048382), but this causes a failure in test_ComplexRegion_union : 

``` python
c1 = ComplexRegion(Interval(0, 1)*Interval(0, 2*S.Pi), polar=True)
#...
assert c1.union(Interval(2, 4)) == Union(c1, Interval(2, 4), evaluate=False)
```

I think sympy is not being consistent here. Tests expect treating real numbers as complex numbers when making union with S.Complexes, yet also expect them to be completely separate when making union with _any_ other subset of complex numbers (In fact, at the moment, S.Complexes in unions behaves a bit like S.UniversalSet).

``` python
>>>Interval(0,1).union(ComplexRegion(Interval(-5,5)**2))
[0,1] ∪ { x + yi | x,y ∈ [−5,5]^2 }
>>>Interval(0,1).union(S.Complexes)
ℂ
>>> Union(S.Complexes, FiniteSet(x))
ℂ
```

Should I treat real numbers as complex ones in my fix? If I do, I'd need to modify the aforementioned test.

If the new test output is still mathematically correct, then just change the test. union returning Union is the default behavior. If it can return something more specific instead, that is preferred.

Commit message: Add unit test for issue 11730

    Modified test test_ComplexRegion_union to be consistent with other
    tests.

Patch to review:
diff --git a/sympy/sets/fancysets.py b/sympy/sets/fancysets.py
index a009f3f7ab95..5f88e53a5edd 100644
--- a/sympy/sets/fancysets.py
+++ b/sympy/sets/fancysets.py
@@ -1409,8 +1409,6 @@ def _intersect(self, other):
                 return ComplexRegion(new_r_interval*new_theta_interval,
                                     polar=True)
 
-        if other is S.Reals:
-            return other
 
         if other.is_subset(S.Reals):
             new_interval = []
@@ -1443,8 +1441,13 @@ def _union(self, other):
             elif self.polar and other.polar:
                 return ComplexRegion(Union(self.sets, other.sets), polar=True)
 
-        if self == S.Complexes:
-            return self
+        # treat a subset of Reals as complex numbers
+        if other.is_subset(S.Reals):
+            as_complex = other * FiniteSet(0)
+            if as_complex.is_subset(self.sets):
+                return self
+            else:
+                return ComplexRegion(Union(self.sets, as_complex), polar=self.polar)
 
         return None
 
diff --git a/sympy/sets/tests/test_fancysets.py b/sympy/sets/tests/test_fancysets.py
index 0d26c0b65032..45832cde5781 100644
--- a/sympy/sets/tests/test_fancysets.py
+++ b/sympy/sets/tests/test_fancysets.py
@@ -5,7 +5,7 @@
                              Intersection)
 from sympy.simplify.simplify import simplify
 from sympy import (S, Symbol, Lambda, symbols, cos, sin, pi, oo, Basic,
-                   Rational, sqrt, tan, log, exp, Abs, I, Tuple)
+                   Rational, sqrt, tan, log, exp, Abs, I, Tuple, eye)
 from sympy.utilities.iterables import cartes
 from sympy.utilities.pytest import XFAIL, raises
 from sympy.abc import x, y, z, t
@@ -593,8 +593,8 @@ def test_ComplexRegion_union():
     assert c5.union(c6) == ComplexRegion(p3)
     assert c7.union(c8) == ComplexRegion(p4)
 
-    assert c1.union(Interval(2, 4)) == Union(c1, Interval(2, 4), evaluate=False)
-    assert c5.union(Interval(2, 4)) == Union(c5, Interval(2, 4), evaluate=False)
+    assert c1.union(Interval(2, 4)) == Union.reduce(Union(c1, Interval(2, 4), evaluate=False).args)
+    assert c5.union(Interval(2, 4)) == Union.reduce(Union(c5, Interval(2, 4), evaluate=False).args)
 
 
 def test_ComplexRegion_measure():
@@ -698,3 +698,13 @@ def test_issue_11732():
     assert (pointComplex in S.Naturals0) == False
     assert (pointComplex in S.Integers) == False
     assert (pointComplex in S.Complexes) == True
+
+
+def test_issue_11730():
+    unit = Interval(0, 1)
+    square = ComplexRegion(unit ** 2)
+
+    assert Union(S.Complexes, FiniteSet(oo)) != S.Complexes
+    assert Union(S.Complexes, FiniteSet(eye(4))) != S.Complexes
+    assert Union(unit, square) == square
+    assert Intersection(S.Reals, square) == unit


Code review comments:
diff_hunk: @@ -1443,8 +1441,13 @@ def _union(self, other):
             elif self.polar and other.polar:
                 return ComplexRegion(Union(self.sets, other.sets), polar=True)
 
-        if self == S.Complexes:
-            return self
+        # treat a subset of Reals as complex numbers
+        if other.is_subset(S.Reals):
path: sympy/sets/fancysets.py
text: @user1:
The method could probably check this first and coerce other into a ComplexRegion before asking `if other.is_ComplexRegion`.

@author:
I don't think converting other to ComplexRegion is necessary, since under the `if other.is_ComplexRegion:` we are only using `other.sets` which just returns set passed in constructor, and this way we don't incur overhead of ComplexRegion's constructor.

If you think this makes logic of what's happening clearer, I'll change it.

@user1:
>  this makes logic of what's happening clearer

Yes, that is what I think. We should avoid having to use any knowledge of the internal representation of ComplexRegion. Hence also the conversion should be carried out by ComplexRegion despite of some overhead. That could be done by overloading `ComplexRegion.__new__` to accept a subset of Reals, or, maybe preferably, by a new method `ComplexRegion.from_real`.