Enforce elementwise mul for explicit non-matrix

[mloubout]

References to other Issues or PRs

Brief description of what is fixed or changed

This PR slightly increase the checks in a Matrix mul so that if multiplied by an object that is explicitly not a matrix, it defaults back to elementwise.

Other comments

This is an issue we run into in Devito, as we have sympy.Function and sympy.Symbols object but with associated data and shape. These objects are non-matrix (will be made explicit with upcoming tensor algebra update) but error when we try to multiply a Matrix.

for example:

A = Matrix([[1, 2], [3, 4]])
b = devito.Function(name="b", grid=Grid((10, 10)))
b* A # should return the Matrix [[ b, 2 *b], [3*b, 4*b]]

error as b.shape = (10, 10) (so has shape) and len(b.shape) == 2 so mul thinks b is a matrix and tries to do a MatMat mul with incompatible shapes.

Release Notes

• matrices
  • Explicit non-matrix are treated as scalar

[sympy-bot]

✅

Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• matrices
  • Explicit non-matrix are treated as scalar (#19533 by @mloubout)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- 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. -->


#### Brief description of what is fixed or changed

This PR slightly increase the checks in a Matrix mul so that if multiplied by an object that is explicitly not a matrix,  it defaults back to elementwise.

#### Other comments

This is an issue we run into in [Devito](https://github.com/devitocodes/devito), as we have `sympy.Function` and `sympy.Symbols` object but with associated data and shape. These objects are non-matrix (will be made explicit with upcoming tensor algebra update) but error when we try to multiply a Matrix.

for example:

```
A = Matrix([[1, 2], [3, 4]])
b = devito.Function(name="b", grid=Grid((10, 10)))
b* A # should return the Matrix [[ b, 2 *b], [3*b, 4*b]]
```
error as `b.shape = (10, 10)` (so has shape) and `len(b.shape) == 2` so mul thinks b is a matrix and tries to do a MatMat mul with incompatible shapes.

#### 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
  * Explicit non-matrix are treated as scalar

<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[sylee957]

Why should the default values of is_Matrix and is_MatrixLike be True?

[mloubout]

Why should the default values of is_Matrix and is_MatrixLike be True?

This checks if other is explicitly not a Matrix, so the default is that it's a MAtrix (true) unless said so.

[sylee957]

Okay, I get that they are set true only to skip the shape check,
but I think that they should be getattr(other, 'is_Matrix', True) or getattr(other, 'is_MatrixLike', True) rather than and because it was the consistent behavior that the objects with either is_Matrix=True or is_MatrixLike=True is multiplied as a matrix.

[mloubout]

That's a fair point but I'm unsure which one is better.
IMO if either is true (i.e or) then it is a matrix so this should not be considered a non-matrix. So the and is really a "if not a matrix in any way".

[codecov]

Codecov Report

Merging #19533 into master will increase coverage by 0.013%.
The diff coverage is 66.666%.

@@              Coverage Diff              @@
##            master    #19533       +/-   ##
=============================================
+ Coverage   75.651%   75.665%   +0.013%     
=============================================
  Files          652       654        +2     
  Lines       169712    169941      +229     
  Branches     40051     40066       +15     
=============================================
+ Hits        128390    128586      +196     
- Misses       35720     35738       +18     
- Partials      5602      5617       +15     

[sylee957]

For consistency with the old behavior, I don't think that it's better to silently use and because

from sympy import *

A = Matrix([[1, 2], [3, 4]])

class foo:
    is_Matrix=False
    is_MatrixLike=True
    shape = (1, 1)
    rows, cols = shape
    
    def __getitem__(self, ij):
        i, j = ij
        return {(0, 0): 1}[i, j]

A * foo()

now attempts to multiply with invalid shape, rather than raising ShapeError like before

I'm not sure that is_MatrixLike was widely used or well tested in sympy before, but it's time to add the comprehensive tests to define the behavior when is_Matrix and is_MatrixLike are combined.

[mloubout]

➜  sympy git:(matforce) grep -rn 'is_MatrixLike' sympy    
sympy/matrices/common.py:2488:        if getattr(other, 'is_MatrixLike', False):
sympy/matrices/common.py:2500:        if not getattr(other, 'is_Matrix', False) and not getattr(other, 'is_MatrixLike', False):
sympy/matrices/common.py:2557:            getattr(other, 'is_MatrixLike', True)):
sympy/matrices/common.py:2570:        if getattr(other, 'is_MatrixLike', False):
sympy/matrices/common.py:2717:        if not getattr(other, 'is_Matrix', False) and not getattr(other, 'is_MatrixLike', False):
sympy/matrices/common.py:2729:            getattr(other, 'is_MatrixLike', True)):
sympy/matrices/common.py:2737:        if getattr(other, 'is_MatrixLike', False):
sympy/matrices/common.py:2779:    is_MatrixLike = True
sympy/matrices/common.py:2889:    is_MatrixLike = True
sympy/matrices/common.py:2914:    if getattr(mat, 'is_Matrix', False) or getattr(mat, 'is_MatrixLike', False):
sympy/matrices/common.py:2918:    if not (getattr(mat, 'is_MatrixLike', True) or getattr(mat, 'is_MatrixLike', True)):

So _MatrixWrapper and MinimalMAtrix are the only ones using it. I think is_MatrixLike should be dropable and make all of this more robust with only is_Matrix will look into it in the morning.

A * foo()
This one does indeed behave very badly

[sylee957]

Although is_MatrixLike wasn't implemented well, I think that it's too hasty to remove now than following our deprecation policy.
It was named as a public method and at least your downstream usage gives a good example that this can break some downstream projects.

[mloubout]

Very fair point, my bad.

I reverted it to my orginal changes and switched to or instead of and. It is a bit looser but still controllable if one puts both is_Matrix and is_MatrixLike to False to enforce scalar behavior.

I added a "foo:" test for it and I can add the one above to make sure it still fails as expected.