Repository: sympy/sympy
Pull request: Matrix hstack/vstack improved
fixes #11944 


Problem statement:
matrix vstack/hstack can fail with immutable matrix as first argument
````
>>> A = Matrix([[1, 2], [pi, 4]])
>>> AIm = sympify(A)

>>> type(A)
sympy.matrices.dense.MutableDenseMatrix

>>> type(AIm)
sympy.matrices.immutable.ImmutableMatrix

>>> Matrix.vstack(A, AIm)
⎡1  2⎤
⎢    ⎥
⎢π  4⎥
⎢    ⎥
⎢1  2⎥
⎢    ⎥
⎣π  4⎦

>>> Matrix.vstack(AIm, A)
('hello vstack, cls=', <class 'sympy.matrices.dense.MutableDenseMatrix'>)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-37-aec335eeafd0> in <module>()
----> 1 Matrix.vstack(AIm, A)

/home/cbm/work/octsympy/octsympy.git/inst/sympy/matrices/matrices.py in vstack(cls, *args)
   4541         """
-> 4542         return reduce(cls.col_join, args)
   4543 
   4544     def xreplace(self, rule):  # should mirror core.basic.xreplace

TypeError: unbound method col_join() must be called with MutableDenseMatrix instance as first argument (got ImmutableMatrix instance instead)
````
Note it will work if one uses `MatrixBase.vstack`.

I don't think this is really about immutable matrices; its about `cls.col_join` not being the right method, depending on what is in `args`.

One way to fix this would be to take `cls` from the type of `args[0]` instead:
````
kls = type(args[0])
reduce(kls.col_join, args)
````
Although I'm not sure that's quite right.

Another option is to avoid `reduce` and use:
````
M = args.pop(0)
for i = range(len(args)):
    M = M.col_join(args.pop(0))
````
(or more pythonic version thereof).

Whatever we do `hstack` needs the same fix.

Hints from issue discussion:
I want to try this. 
@cbm755 this the output in my system and seems to be fine.
```
In [36]: A=Matrix([[1,2],[pi,4]])

In [37]: AIm=sympify(A)

In [38]: type(A)
Out[38]: sympy.matrices.dense.MutableDenseMatrix

In [39]: type(AIm)
Out[39]: sympy.matrices.immutable.ImmutableMatrix

In [40]: Matrix.vstack(AIm,A)
Out[40]:
?1  2?
?    ?
?π  4?
?    ?
?1  2?
?    ?
?π  4?

In [41]: Matrix.hstack(AIm,A)
Out[41]:
?1  2  1  2?
?          ?
?π  4  π  4?
```
Thanks for checking.  I've tried it and I can reproduce with Python 2 on both Sympy 1.0.1 and the current git master.  But on Python 3, it works as you show: nice catch.


> I want to try this.

Go for it.  It should be an easy fix, I'm just not enough of a Python expert to know which fix is the best.  Please add a test and read the https://github.com/sympy/sympy/wiki/Development-workflow.
Hi @Abdullahjavednesar . Let me know in case you're not working on it :)
This is fixed in https://github.com/sympy/sympy/pull/11592 , for what it's worth :-)
@siefkenj that's great!  I wondered when I saw your refactor but I haven't had a chance to look yet.  Thanks.

@Abdullahjavednesar if that PR goes in first, we'll need to update your PR #11951 to just add tests---this issue will still need tests even if its fixed elsewhere.

Commit message: Tests corrected

Patch to review:
diff --git a/sympy/matrices/matrices.py b/sympy/matrices/matrices.py
index 2fd4efa8ca06..edd9d9fd6008 100644
--- a/sympy/matrices/matrices.py
+++ b/sympy/matrices/matrices.py
@@ -2035,7 +2035,8 @@ def hstack(cls, *args):
         [1, 0, 2, 0],
         [0, 1, 0, 2]])
         """
-        return reduce(cls.row_join, args)
+        kls = type(args[0])
+        return reduce(kls.row_join, args)
 
     def integrate(self, *args):
         """Integrate each element of the matrix.
@@ -4531,7 +4532,8 @@ def vstack(cls, *args):
         [2, 0],
         [0, 2]])
         """
-        return reduce(cls.col_join, args)
+        kls = type(args[0])
+        return reduce(kls.col_join, args)
 
     def xreplace(self, rule):  # should mirror core.basic.xreplace
         """Return a new matrix with xreplace applied to each entry.
diff --git a/sympy/matrices/tests/test_matrices.py b/sympy/matrices/tests/test_matrices.py
index 8c2481580650..7a2c30a50473 100644
--- a/sympy/matrices/tests/test_matrices.py
+++ b/sympy/matrices/tests/test_matrices.py
@@ -2378,6 +2378,13 @@ def test_issue_5320():
         [0, 1, 0, 2]
     ])
 
+def test_issue_11944():
+    A = Matrix([[1]])
+    AIm = sympify(A)
+    assert Matrix.hstack(AIm, A) == Matrix([[1, 1]])
+    assert Matrix.vstack(AIm, A) == Matrix([
+        [1, ],
+        [1, ]])
 
 def test_cross():
     a = [1, 2, 3]


Code review comments:
diff_hunk: @@ -2378,6 +2378,13 @@ def test_issue_5320():
         [0, 1, 0, 2]
     ])
 
+def test_issue_11944():
+    A = Matrix([[1]])
+    AIm = sympify(A)
+    assert Matrix.hstack(AIm, A) == Matrix([[1, 1]])
+    assert Matrix.vstack(AIm, A) == Matrix([
+        [1, ],
+        [1, ]])
path: sympy/matrices/tests/test_matrices.py
text: @user1:
`== Matrix([[1], [1]])` please

@author:
Okay