Repository: sympy/sympy
Pull request: Fix Rationals.boundary, Rationals.is_open, Rationals.is_closed

<!-- 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. -->
Fixes #18134

#### Brief description of what is fixed or changed
This commit fixes #18134 by
- correcting `Rationals.boundary`,
- improving the `is_open` logic by using `is_empty`
  (previously `is_open` could never return `False`),
- updating the docstrings of `is_open` and `is_closed`.

It also makes a few related tests stricter by changing
`assert not ...` to `assert ... is False`.


#### 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 -->
* sets
  * Fixed `Rationals.boundary` to be `Reals`. (`is_open` and `is_closed` now both return `False`.)
  * `is_open` logic is improved.
<!-- END RELEASE NOTES -->


Problem statement:
Are `is_closed, is_open` of `S.Rationals`'s properties valid?
currently, there properties `is_closed, is_open` aren't initialized .

```python
from sympy import S

S.Rationals.is_closed, S.Rationals.is_open
> True, None
```

if there properties are thought wheather Q(S.Rationals) is open or closed set in R (S.Reals), should return `is_open=False` and `is_closed=False`.
if use there properties as undefined, should return a exception `Not Implemented Error` ?

Hints from issue discussion:
Generally in SymPy `is_` properties return fuzzy-bools which use 3-way logic: True, False or None. None means that the the answer to the question is not known. Often that is because the code to answer the question has not been written/implemented.

Right now the `is_closed` property returns `self.boundary.is_subset(self)` and for rationals we have
```
In [1]: Rationals                                                                                                                                             
Out[1]: ℚ

In [2]: Rationals.boundary                                                                                                                                    
Out[2]: ℚ

In [3]: Rationals.boundary.is_subset(Rationals)                                                                                                               
Out[3]: True
```
Either Rationals.boundary should not be the Rationals or the `is_closed`
 method should be changed in general.

The `is_open` method returns True if `Intersection(self, self.boundary)` gives the empty set and None otherwise. That's why we get a None in this case.
> Either Rationals.boundary should not be the Rationals or the `is_closed`
> method should be changed in general.

I'd have expected the open/closed answers for subsets of the reals to be based on the topology on R. Otherwise (i.e. if the Rationals, Integers etc. were considered to be their own topological spaces) they'd all simply have to be considered open and closed by definition.

So that would mean `Rationals.boundary == Reals` (difference of closure(Q)=R and interior(Q)=empty).
It follows that Q is neither open nor closed.

I'm not a topologist though and my topology courses date back some time...

Do note that the docs for `is_open` and `is_closed` don't actually specify the topology, so that should be amended too.
@oscarbenjamin thx. 

>Generally in SymPy is_ properties return fuzzy-bools which use 3-way logic

i knew for first time. is it written in doc?



because of being different whether the set is closed or open  by universal set,  
may be have to change there properties to methods with arg `universal_set`.

Commit message: Fix Rationals.boundary, Rationals.is_open, Rationals.is_closed

This commit fixes #18134 by
- correcting Rationals.boundary,
- improving the `is_open` logic by using `is_empty`
  (previously `is_open` could never return `False`),
- updating the docstrings of `is_open` and `is_closed`.

It also makers a few related tests stricter by changing
`assert not ...` to `assert ... is False`.

Patch to review:
diff --git a/sympy/sets/fancysets.py b/sympy/sets/fancysets.py
index f74693b4705c..0e859867eed2 100644
--- a/sympy/sets/fancysets.py
+++ b/sympy/sets/fancysets.py
@@ -66,7 +66,7 @@ def __iter__(self):
 
     @property
     def _boundary(self):
-        return self
+        return S.Reals
 
 
 class Naturals(with_metaclass(Singleton, Set)):
diff --git a/sympy/sets/sets.py b/sympy/sets/sets.py
index 96b6751597a0..c9aa9adb0407 100644
--- a/sympy/sets/sets.py
+++ b/sympy/sets/sets.py
@@ -555,25 +555,29 @@ def boundary(self):
     def is_open(self):
         """
         Property method to check whether a set is open.
+
         A set is open if and only if it has an empty intersection with its
-        boundary.
+        boundary. The openness of a subset of the reals is determined with
+        respect to R and its standard topology.
 
         Examples
         ========
         >>> from sympy import S
         >>> S.Reals.is_open
         True
+        >>> S.Rationals.is_open
+        False
         """
-        if not Intersection(self, self.boundary):
-            return True
-        # We can't confidently claim that an intersection exists
-        return None
+        return Intersection(self, self.boundary).is_empty
 
     @property
     def is_closed(self):
         """
-        A property method to check whether a set is closed. A set is closed
-        if its complement is an open set.
+        A property method to check whether a set is closed.
+
+        A set is closed if its complement is an open set. The closedness of a
+        subset of the reals is determined with respect to R and its standard
+        topology.
 
         Examples
         ========
diff --git a/sympy/sets/tests/test_fancysets.py b/sympy/sets/tests/test_fancysets.py
index 6d35a0731044..140b65dbc65f 100644
--- a/sympy/sets/tests/test_fancysets.py
+++ b/sympy/sets/tests/test_fancysets.py
@@ -29,6 +29,8 @@ def test_naturals():
     assert N.intersect(Interval(-5, 5, True, True)) == Range(1, 5)
 
     assert N.boundary == N
+    assert N.is_open == False
+    assert N.is_closed == True
 
     assert N.inf == 1
     assert N.sup is oo
@@ -71,6 +73,8 @@ def test_integers():
     assert Z.sup is oo
 
     assert Z.boundary == Z
+    assert Z.is_open == False
+    assert Z.is_closed == True
 
     assert Z.as_relational(x) == And(Eq(floor(x), x), -oo < x, x < oo)
 
@@ -948,7 +952,10 @@ def test_Rationals():
     r = symbols('r', rational=True)
     assert r in S.Rationals
     raises(TypeError, lambda: x in S.Rationals)
-    assert S.Rationals.boundary == S.Rationals
+    # issue #18134:
+    assert S.Rationals.boundary == S.Reals
+    assert S.Rationals.is_open == False
+    assert S.Rationals.is_closed == False
 
 
 def test_imageset_intersection():
diff --git a/sympy/sets/tests/test_sets.py b/sympy/sets/tests/test_sets.py
index f2561a6d3860..e287526ebdec 100644
--- a/sympy/sets/tests/test_sets.py
+++ b/sympy/sets/tests/test_sets.py
@@ -1124,16 +1124,16 @@ def test_boundary_ProductSet_line():
 
 
 def test_is_open():
-    assert not Interval(0, 1, False, False).is_open
-    assert not Interval(0, 1, True, False).is_open
-    assert Interval(0, 1, True, True).is_open
-    assert not FiniteSet(1, 2, 3).is_open
+    assert Interval(0, 1, False, False).is_open is False
+    assert Interval(0, 1, True, False).is_open is False
+    assert Interval(0, 1, True, True).is_open is True
+    assert FiniteSet(1, 2, 3).is_open is False
 
 
 def test_is_closed():
-    assert Interval(0, 1, False, False).is_closed
-    assert not Interval(0, 1, True, False).is_closed
-    assert FiniteSet(1, 2, 3).is_closed
+    assert Interval(0, 1, False, False).is_closed is True
+    assert Interval(0, 1, True, False).is_closed is False
+    assert FiniteSet(1, 2, 3).is_closed is True
 
 
 def test_closure():


Code review comments:
diff_hunk: @@ -555,25 +555,29 @@ def boundary(self):
     def is_open(self):
         """
         Property method to check whether a set is open.
+
         A set is open if and only if it has an empty intersection with its
-        boundary.
+        boundary. The openness of a subset of the reals is determined with
path: sympy/sets/sets.py
text: @user1:
I think that this could be more explicit. Maybe "a subset A of the reals is open if and only if each one of its points is contained in an open interval that is a subset of A". (This is equivalent to saying that A is the union of a family of open intervals.)

@author:
Thanks for the suggestion, I've changed it.
I think I'll also rephrase the `is_closed` docstring to:
"A set is closed if its complement is an open set. In a metric space, in particular the reals, a set is closed if and only if it contains all of its limit points."

And I'll also change the added union test to test the actual results instead of only the fact that they're simplified.