Repository: sympy/sympy
Pull request: Added test case for GramSchimdt
<!-- 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 #9488

#### Brief description of what is fixed or changed
Added required test case.

#### 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 -->
NO ENTRY
<!-- END RELEASE NOTES -->

Problem statement:
FiniteSets can be filled with other objects than numbers, but this is not supported by other classes.
```
>>> FiniteSet([Matrix([1,2,0]), Matrix([1,3,0])])
{Matrix([
[1],
[2],
[0]]), Matrix([
[1],
[3],
[0]])}
```

But

```
>>> L= FiniteSet([Matrix([1,2,0]), Matrix([1,3,0])])
>>> GramSchmidt(L)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.7/dist-packages/sympy/matrices/dense.py", line 1531, in GramSchmidt
    tmp = vlist[i]
TypeError: 'FiniteSet' object does not support indexing
>>>
```

In my opinion a set is a storage for all mathematical objects, and therefore it should also fulfill this job in sympy, or am I wrong ?

So (in my opinion), there should be added an exception to the FiniteSet class, so that it only can be filled with numbers for the moment, and, as an other issue, the set class should be improved, so that it could contain all kind of mathematical objects.

Hints from issue discussion:
I agree about adding the exception, since it's clearly not supported yet in other classes.

Hi,

in the tests_sets.py the following line can be found:

```
s = FiniteSet((1, 2), Float, A, -5, x, 'eggs', x**2, Interval)
```

so this should be fixed, too, and we should test if

```
FiniteSet(3,2,complex('5+j'), Number(3), 3.47342, Symbol('a'), Matrix([3,4]), 'eggs')
```

raises TypeError,

or am I wrong? 

Edit/PS:
In my opinion subclasses of int, float, complex (since the FiniteSet constructor sympyfies his arguments), sympy.numbers.Number and sympy.symbols.Symbol should be allowed, but nothing else for the moment.

I don't think the issue here is that of objects other than numbers:

```
>>> from sympy import FiniteSet, GramSchmidt
>>> s = FiniteSet(1, 2, 3)
>>> s
{1, 2, 3}
>>> GramSchmidt(s)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.4/site-packages/sympy/matrices/dense.py", line 1483, in GramSchmidt
    tmp = vlist[i]
TypeError: 'FiniteSet' object does not support indexing

```

Ok,

then the Problem lies in GramSchmidt. Sorry, I should have seen that. 

In my opinion applying GramSchmidt on sets is very natural, since Bases are sets. 
However, iterating trough a set, by mapping elements of a set to definite integer numbers, not really makes sense (since there is no canonic isomorphism :D). 

Therefore i would suggest, that we should (if you share my opinion, that GramSchmidt should work with sets, too) improve the GramSchmidt algorithm by iterating in the sense "for i in FiniteSet(...)", since this works with sets:

```
>>> for i in FiniteSet(Matrix([3,4,5]), Matrix([0,8,15])):
...     print(i)
... 
Matrix([
[ 0],
[ 8],
[15]])
Matrix([
[3],
[4],
[5]])
```

For the case of `FiniteSets`, another possible solution can be implementing `__getitem__`. I tried it and I seem to get an answer.

Oops, but args are not ordered. That could be a problem. I don't really have much knowledge about `GramSchmidt`.

The documentation of `GramSchmidt` is a bit confusing.

```
def GramSchmidt(vlist, orthog=False):
    """
    Apply the Gram-Schmidt process to a set of vectors.
```

The docstring speaks of a set of vectors, but the argument is called `vlist` and a list is also returned.
Thus one is expected to call with a list argument (`GramSchmidt(list(L))`).

I think this is a better solution than changing `GramSchmidt` to work with sets, since the Gram-Schmidt process relies on the order of vectors in the sequence. Otherwise the result would be  unpredictable and possibly implementation dependent. 

The expression in the original post here raises now. I think that's because the signature of FiniteSet has changed so:
```
In [13]: L = FiniteSet(Matrix([1,2,0]), Matrix([1,3,0]))                                                 

In [14]: L                                                                                               
Out[14]: 
⎧⎡1⎤  ⎡1⎤⎫
⎪⎢ ⎥  ⎢ ⎥⎪
⎨⎢2⎥, ⎢3⎥⎬
⎪⎢ ⎥  ⎢ ⎥⎪
⎩⎣0⎦  ⎣0⎦⎭

In [15]: GramSchmidt(L)                                                                                  
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-15-e1ef40d009e6> in <module>
----> 1 GramSchmidt(L)

~/current/sympy/sympy.git/sympy/matrices/dense.py in GramSchmidt(vlist, orthonormal)
   1212     m = len(vlist)
   1213     for i in range(m):
-> 1214         tmp = vlist[i]
   1215         for j in range(i):
   1216             tmp -= vlist[i].project(out[j])

TypeError: 'FiniteSet' object does not support indexing

In [16]: GramSchmidt(list(L))                                                                            
Out[16]: 
⎡⎡1⎤  ⎡-2/5⎤⎤
⎢⎢ ⎥  ⎢    ⎥⎥
⎢⎢2⎥, ⎢1/5 ⎥⎥
⎢⎢ ⎥  ⎢    ⎥⎥
⎣⎣0⎦  ⎣ 0  ⎦⎦
```
Everything is fine then except the docstring for Gram-Schmidt which should be updated to say that the input vectors should be in a sequence and that it returns a list (rather than unordered sets).
Marking as easy to fix
On the current master I get:
```
>>> from sympy import *
>>> L = FiniteSet(Matrix([1,2,0]), Matrix([1,3,0]))
>>> L
FiniteSet(Matrix([
[1],
[2],
[0]]), Matrix([
[1],
[3],
[0]]))
>>> GramSchmidt(L)
[Matrix([
[1],
[2],
[0]]), Matrix([
[-2/5],
[ 1/5],
[   0]])]
```
Should this closed with a test case? @oscarbenjamin

Commit message: Added test case for GramSchimdt

Patch to review:
diff --git a/sympy/matrices/tests/test_matrices.py b/sympy/matrices/tests/test_matrices.py
index 101a16d977d3..8e3b9ae2d16f 100644
--- a/sympy/matrices/tests/test_matrices.py
+++ b/sympy/matrices/tests/test_matrices.py
@@ -2276,6 +2276,11 @@ def test_GramSchmidt():
     assert GramSchmidt([Matrix([3, 1]), Matrix([2, 2])], True) == [
         Matrix([3*sqrt(10)/10, sqrt(10)/10]),
         Matrix([-sqrt(10)/10, 3*sqrt(10)/10])]
+    # Issue #9488
+    from sympy import FiniteSet
+    L = FiniteSet(Matrix([1]))
+    print(GramSchmidt(L))
+    assert GramSchmidt(L) == [Matrix([[1]])]
 
 
 def test_casoratian():


Code review comments:
diff_hunk: @@ -2276,6 +2276,11 @@ def test_GramSchmidt():
     assert GramSchmidt([Matrix([3, 1]), Matrix([2, 2])], True) == [
         Matrix([3*sqrt(10)/10, sqrt(10)/10]),
         Matrix([-sqrt(10)/10, 3*sqrt(10)/10])]
+    # Issue #9488
+    from sympy import FiniteSet
+    L = FiniteSet(Matrix([1]))
+    print(GramSchmidt(L))
path: sympy/matrices/tests/test_matrices.py
text: @user1:
Imports should go at the top of the file.

There should be no print here.

I think it's better to include a link to the issue in a comment rather than just the issue number.

@author:
Okay