Fix sparse min/max to return sparse matrix #3536
Conversation
takagi
force-pushed
the
sparse-min-max-compat
branch
from
dff1062
to
3d30465
Compare
|
|
||
| return value | ||
| mask = value != 0 | ||
| major_index = cupy.compress(mask, major_index) |
There was a problem hiding this comment.
OK to leave the following issues because the PR is a bugfix.
- For efficiency, it is possible that
mat._minor_reduceshould compute themajor_indexand the compressedvalue. coo_matrix(D)could be implemented, which is discussed at missing constructor forscipy.sparse.csr_matrix#3289 (comment)
There was a problem hiding this comment.
I'll revisit these points in another PR!
| assert numpy.array_equal(da_cupy_values, da_scipy_values) | ||
| da_cupy_values = cp_matrix.min(axis=0) | ||
| da_scipy_values = dm_data.min(axis=0) | ||
| testing.assert_array_equal(da_scipy_values.A, da_scipy_values.A) |
There was a problem hiding this comment.
testing.assert_array_equal does not support sparse, while testing.numpy_cupy_array_equal and testing.numpy_cupy_allclose do.
There was a problem hiding this comment.
I make the tests use testing.numpy_cupy_arrray_equal. Would you check them? To test nonzero parameter, which SciPy does not have yet scipy/scipy#11899 and will be marked as experimental in #3583, I supply a bit messy SciPy inputs.
takagi
force-pushed
the
sparse-min-max-compat
branch
from
2bcaaa5
to
ecbb7a7
Compare
|
Jenkins, test this please. |
|
Successfully created a job for commit 5431fc6: |
|
Jenkins CI test (for commit 5431fc6, target branch master) failed with status FAILURE. |
|
Thanks! I'll fix the SciPy input to make it work in case |
|
Jenkins, test this please. |
|
Successfully created a job for commit 570ddcc: |
|
Jenkins CI test (for commit 570ddcc, target branch master) succeeded! |
Close #3504.
This PR fixes sparse min/max to return a value as a sparse matrix. I will parameterize the test in another PR.