Repository: sympy/sympy
Pull request: NotImplemented for matrix and tensor addition
<!-- 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 #18956

#### Brief description of what is fixed or changed
Added not implemented  for tensor + matrix, and matrix + tensor.

#### Other comments
Earlier Matrix + Array gave result as a matrix and Array + Matrix gave Type Error . Now both functs would return Not Implemented.

#### 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 -->
* matrices
     * Adding an array and a matrix now consistently gives TypeError.
<!-- END RELEASE NOTES -->

Problem statement:
Addition of tensor and matrix with same shape
Little example:

```
import sympy as sp
A = sp.Array([[1, 2], [3, 4]])
B = sp.eye(2)
```

```
A + B

---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-32-151064de832d> in <module>
----> 1 A + B

/usr/local/lib/python3.7/dist-packages/sympy/tensor/array/ndim_array.py in __add__(self, other)
    370 
    371         if not isinstance(other, NDimArray):
--> 372             raise TypeError(str(other))
    373 
    374         if self.shape != other.shape:

TypeError: Matrix([[1, 0], [0, 1]])
```

Whereas:

```
B + A
```

works as expected.

Hints from issue discussion:
Instead of raising `TypeError` it should return `NotImplemented`. Both examples should raise though so `Matrix.__add__` should also return `NotImplemented`.
I think that one of the problem in the sympy codebase are there are so many different array classes inter-operating badly.
I may have different ideas like `Matrix + Array` should always return `Array` if it is two dimensional.
> Instead of raising `TypeError` it should return `NotImplemented`. Both examples should raise though so `Matrix.__add__` should also return `NotImplemented`.

It should return NotImplemented or shall give NotImplementedError?
NotImplemented is a reserved error for builtin operator overrides and cannot be used interchangably with NotImplementedError
https://docs.python.org/3/library/constants.html#NotImplemented
In matrix __add__ method it shows matrix like objects can be added which implies matrix + array = matrix
Then similarly array + matrix shouldn't be array?

Commit message: NotImplemented for matrix and tensor addition and vice versa

Patch to review:
diff --git a/sympy/matrices/common.py b/sympy/matrices/common.py
index 44a3cdc795cf..bbe1c4a69ddf 100644
--- a/sympy/matrices/common.py
+++ b/sympy/matrices/common.py
@@ -26,6 +26,7 @@
 from sympy.utilities.exceptions import SymPyDeprecationWarning
 from sympy.utilities.iterables import flatten
 from sympy.utilities.misc import filldedent
+from sympy.tensor.array import NDimArray
 
 from .utilities import _get_intermediate_simp_bool
 
@@ -2535,6 +2536,8 @@ def __abs__(self):
 
     @call_highest_priority('__radd__')
     def __add__(self, other):
+        if isinstance(other, NDimArray):
+            return NotImplemented
         """Return self + other, raising ShapeError if shapes don't match."""
         other = _matrixify(other)
         # matrix-like objects can have shapes.  This is
diff --git a/sympy/matrices/tests/test_commonmatrix.py b/sympy/matrices/tests/test_commonmatrix.py
index 811d9cd96200..84e8378e0934 100644
--- a/sympy/matrices/tests/test_commonmatrix.py
+++ b/sympy/matrices/tests/test_commonmatrix.py
@@ -20,6 +20,7 @@
 from sympy.polys.polytools import Poly
 from sympy.utilities.iterables import flatten
 from sympy.testing.pytest import raises, XFAIL, warns_deprecated_sympy
+from sympy import Array
 
 from sympy.abc import x, y, z
 
@@ -1059,3 +1060,8 @@ class Foo(ImmutableDenseMatrix):
 
     assert isinstance(c, Foo)
     assert c == Matrix([[7, 10], [15, 22]])
+
+def test_issue_18956():
+    A = Array([[1, 2], [3, 4]])
+    B = Matrix([[1,2],[3,4]])
+    raises(TypeError, lambda: B + A)
diff --git a/sympy/tensor/array/ndim_array.py b/sympy/tensor/array/ndim_array.py
index b2288b9e942a..d7b63776344f 100644
--- a/sympy/tensor/array/ndim_array.py
+++ b/sympy/tensor/array/ndim_array.py
@@ -315,8 +315,11 @@ def f(sh, shape_left, i, j):
 
     def __add__(self, other):
         from sympy.tensor.array.arrayop import Flatten
+        from sympy.matrices import Matrix
 
         if not isinstance(other, NDimArray):
+            if isinstance(other, Matrix):
+                return NotImplemented
             raise TypeError(str(other))
 
         if self.shape != other.shape:
diff --git a/sympy/tensor/array/tests/test_ndim_array.py b/sympy/tensor/array/tests/test_ndim_array.py
index 9a097a3fb86b..2aa1b92e73a6 100644
--- a/sympy/tensor/array/tests/test_ndim_array.py
+++ b/sympy/tensor/array/tests/test_ndim_array.py
@@ -6,6 +6,7 @@
 )
 from sympy.abc import x, y
 
+from sympy.matrices import Matrix
 
 array_types = [
     ImmutableDenseNDimArray,
@@ -42,3 +43,8 @@ def test_issue_18361():
     assert simplify(A) == Array([0])
     assert simplify(B) == Array([1, 0])
     assert simplify(C) == Array([x + 1, sin(2*x)])
+
+def test_issue_18956():
+    A = Array([[1, 2], [3, 4]])
+    B = Matrix([[1,2],[3,4]])
+    raises(TypeError, lambda: A+B)


Code review comments:
diff_hunk: @@ -2535,6 +2536,8 @@ def __abs__(self):
 
     @call_highest_priority('__radd__')
     def __add__(self, other):
+        if isinstance(other, NDimArray):
+            return NotImplemented
         """Return self + other, raising ShapeError if shapes don't match."""
path: sympy/matrices/common.py
text: @user1:
There should not be code before the docstring.

Also this should be phrased as:
```
if not isinstance(other, ...):
     return NotImplemented
```
The method should return `NotImplemented` unless it explicitly knows how to interoperate with other object.

@author:
In this case I think it should be if isinstance(other, NdimArray) because matrix class is meant to work work with all objects having similar shape, if not isinstance will make matrix class work only with matrix not any other object of similar shape. I think only Matrix and Array addition should return NotImplemented

@user1:
What does "other object of similar shape" mean?

In general `NotImplemented` should be returned except for a specific list of allowed types.

@author:
Can you please list the allowed types?



@author:
> What does "other object of similar shape" mean?

Matrix like object

diff_hunk: @@ -315,8 +315,11 @@ def f(sh, shape_left, i, j):
 
     def __add__(self, other):
         from sympy.tensor.array.arrayop import Flatten
+        from sympy.matrices import Matrix
 
         if not isinstance(other, NDimArray):
+            if isinstance(other, Matrix):
+                return NotImplemented
             raise TypeError(str(other))
path: sympy/tensor/array/ndim_array.py
text: @user1:
This should always return `NotImplemented`. We shouldn't raise `TypeError` in methods like `__add__` because then it's impossible to define other classes that can interact with this class.

@author:
I didn't add TypeError , fixed it anyway.