Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: A sparse matrix raises a ValueError when __rmul__ with a value as None #15210

Closed
takagi opened this issue Dec 14, 2021 · 16 comments · Fixed by #15232 or #15235
Closed

BUG: A sparse matrix raises a ValueError when __rmul__ with a value as None #15210

takagi opened this issue Dec 14, 2021 · 16 comments · Fixed by #15232 or #15235
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse
Milestone

Comments

@takagi
Copy link
Contributor

takagi commented Dec 14, 2021

Describe your issue.

A sparse matrix multiplied with a value of an unsupported operand type raises TypeError. However, in SciPy 1.8.0rc1, a sparse matrix raises ValueError when it is called __rmul__ with a value that is not interpretable as an array, e.g. None. It should raise TypeError instead as __mul__ does.

Reproducing Code Example

import numpy as np
import scipy.sparse as sp
m = np.eye(3)
m = sp.coo_matrix(m)
None * m

Error message

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ext-mtakagi/.pyenv/versions/nvidia/lib/python3.9/site-packages/scipy/sparse/_base.py", line 610, in __rmul__
    return self._rmul_dispatch(other)
  File "/home/ext-mtakagi/.pyenv/versions/nvidia/lib/python3.9/site-packages/scipy/sparse/_base.py", line 607, in _rmul_dispatch
    return (self.transpose() @ tr).transpose()
  File "/home/ext-mtakagi/.pyenv/versions/nvidia/lib/python3.9/site-packages/scipy/sparse/_base.py", line 618, in __matmul__
    raise ValueError("Scalar operands are not allowed, "
ValueError: Scalar operands are not allowed, use '*' instead

SciPy/NumPy/Python version information

1.8.0rc1 1.22.0rc1 sys.version_info(major=3, minor=9, micro=7, releaselevel='final', serial=0)

@takagi takagi added the defect A clear bug or issue that prevents SciPy from being installed or used as expected label Dec 14, 2021
@WarrenWeckesser
Copy link
Member

Thanks for reporting this bug, @takagi. You are correct, that should be a TypeError.

@WarrenWeckesser WarrenWeckesser added this to the 1.8.0 milestone Dec 14, 2021
@WarrenWeckesser
Copy link
Member

I'm adding new issues that should be fixed for 1.8.0rc2 (or 1.8.0 final) to the 1.8.0 milestone. If a different procedure is preferred, let me know.

@takagi
Copy link
Contributor Author

takagi commented Dec 15, 2021

xref: cupy/cupy#6233

@takagi
Copy link
Contributor Author

takagi commented Dec 15, 2021

Thanks, it is definitely fine for us.

@stefanv
Copy link
Member

stefanv commented Dec 15, 2021

The problem is that None can be coerced into an array:

In [2]: np.array(None)
Out[2]: array(None, dtype=object)

I can add a specific check for None, but it sounds like you want a wider class of objects excluded?

I you only want None to be excluded, I can easily add that back in with a special check.

@stefanv
Copy link
Member

stefanv commented Dec 15, 2021

Here's the offending code from _base.py:

        # If it's a list or whatever, treat it like a matrix
        other_a = np.asanyarray(other)

@toslunar
Copy link
Contributor

How about reverting this line https://github.com/scipy/scipy/pull/14822/files#diff-8aed1ba485f8d01bc74679865b6ca8fa04efde8b6fef5a57f587b61eb7a8734cR607 to

return (self.transpose() * tr).transpose()

or

return self.transpose()._mul_dispatch(tr).transpose()

@stefanv
Copy link
Member

stefanv commented Dec 16, 2021

We cannot revert that line, or it will break for arrays. I think by the time we get to the point where we think array(None, dtype=object) is another array we should interact with, we're in trouble already. I think this goes back to an issue with NumPy where it is a bit over-eager in converting objects to arrays. @seberg Thoughts?

@toslunar
Copy link
Contributor

I got

>>> np.eye(2) * sparse.csr_array(np.arange(6).reshape(2, 3))
array([[0., 1., 2.],
       [3., 4., 5.]])
>>> np.__version__, scipy.__version__
('1.22.0rc2', '1.8.0rc1')

but I expect dense * sparse computes

  • elementwise mul if both operands are "array"
  • matmul if any operand is a "matrix"

@seberg
Copy link
Contributor

seberg commented Dec 16, 2021

I think the problem is that isscalar is not just not very useful. You can't know that the result asarray result won't be 0-D, and if it is you have to go into the normal multiply path. (There might be more subtleties, in that None * m used to fail earlier and not try to go to object arrays, but that seems OK?)
So, you can't commit to using @ quite there, calling the _mul_dispatcher may well be reasonable. By that time, the other object got a chance to deal with it, so converting it to a dense array should not be terrible (but that could be an object array of course).

@stefanv
Copy link
Member

stefanv commented Dec 16, 2021

but I expect dense * sparse computes

  • elementwise mul if both operands are "array"
  • matmul if any operand is a "matrix"

This is a bug, for sure!

@stefanv
Copy link
Member

stefanv commented Dec 16, 2021

OK, figured it out: rmul is missing from the array interface 🙄

stefanv added a commit to stefanv/scipy that referenced this issue Dec 16, 2021
tylerjereddy pushed a commit to tylerjereddy/scipy that referenced this issue Dec 19, 2021
@toslunar
Copy link
Contributor

I'd like to see the issue reopened to clarify the status. The original problem persists in 1.8.0rc2.

Python 3.10.0 (default, Oct 26 2021, 11:19:32) [Clang 11.0.0 (clang-1100.0.33.16)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import numpy as np
>>> import scipy.sparse as sp
>>> m = np.eye(3)
>>> m = sp.coo_matrix(m)
>>> None * m
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File ".../site-packages/scipy/sparse/_base.py", line 610, in __rmul__
    return self._rmul_dispatch(other)
  File ".../site-packages/scipy/sparse/_base.py", line 607, in _rmul_dispatch
    return (self.transpose() @ tr).transpose()
  File ".../site-packages/scipy/sparse/_base.py", line 618, in __matmul__
    raise ValueError("Scalar operands are not allowed, "
ValueError: Scalar operands are not allowed, use '*' instead
>>> np.__version__
'1.22.0'
>>> import scipy; scipy.__version__
'1.8.0rc2'

@stefanv
Copy link
Member

stefanv commented Jan 20, 2022

Yes, sorry, my fix only helped for sparse arrays, not matrices.

@stefanv
Copy link
Member

stefanv commented Jan 20, 2022

Would this be too explicit a fix?

diff --git a/scipy/sparse/_base.py b/scipy/sparse/_base.py
index 0611242e9..47b0ca5fc 100644
--- a/scipy/sparse/_base.py
+++ b/scipy/sparse/_base.py
@@ -604,6 +604,10 @@ class spmatrix:
                 tr = other.transpose()
             except AttributeError:
                 tr = np.asarray(other).transpose()
+
+            if tr.shape == ():
+                raise TypeError('Multiplier is not scalar-like, but also cannot be converted to an array')
+
             return (self.transpose() @ tr).transpose()
 
     def __rmul__(self, other):  # other * self

Cut-and-pastable test code:

import numpy as np
import scipy.sparse as sp
m = np.eye(3)
m = sp.coo_matrix(m)
None * m

@stefanv stefanv reopened this Jan 20, 2022
@toslunar
Copy link
Contributor

It might be possible that tr is a ()-shaped numeric array. While I can't give its exact example for now, there is a "dense array" class that is not covered by _sputils.isdense but np.asarray works.

>>> import numpy as np, scipy.sparse as sparse, torch
>>> sparse._sputils.isscalarlike(torch.tensor(3))
False
>>> np.asarray(torch.tensor(3))
array(3)
>>> type(_)
<class 'numpy.ndarray'>

If the "array" class does not implement transpose, the results differ:

>>> torch.Tensor.transpose = lambda self: "".noattribute  # as if torch.Tensor.transpose were unimplemented
>>> torch.tensor(3) * sparse.csr_matrix([[1, 2], [0, 3]])
<2x2 sparse matrix of type '<class 'numpy.int64'>'
        with 3 stored elements in Compressed Sparse Row format>
>>> _.todense()
matrix([[3, 6],
        [0, 9]])

with SciPy 1.7.3 and

>>> torch.tensor(3) * sparse.csr_matrix([[1, 2], [0, 3]])
ValueError: Scalar operands are not allowed, use '*' instead

with SciPy 1.8.0rc2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect A clear bug or issue that prevents SciPy from being installed or used as expected scipy.sparse
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants