@takagi, have you had a chance to take a look at this fix? 🙂

pfnCI, test this please.

Successfully created a job for commit 5bfbbe4:

• Dashboard for commit 5bfbbe4

Yes, please let me check if the change works on my local as well as running the CI.

Jenkins CI test (for commit 5bfbbe4, target branch master) failed with status FAILURE.

@econtal Sorry for leaving this PR so long. Would you fix the CI failure? It says to import scipy in test_cusparse.py.

Hi @takagi. No worries, thanks for reviewing.

I'm not familiar with cupy testing, I guess the issue was the missing @testing.with_requires('scipy') that I just added.

For this (and eventual further changes), do you prefer amend the commits and force push to clean the git history?

Thanks, you're perfect. And you don't need to clean the git history, it is a reasonable change I think.

pfnCI, test this please.

Successfully created a job for commit 0052756:

• Dashboard for commit 0052756

Jenkins CI test (for commit 0052756, target branch master) succeeded!

I have an issue. With this change, x.col and x.row are processed in-pace by cusparse.xoosortByRow(), while x.val is not processed in-place to be replaced by another ndarray. coosort() looks, I think, to work as an in-place procedure and it would be expected that x.val is sorted in-place as well as x.col and x.val.

How about sorting x.val in-place, for example:

data_orig = x.data.copy()
_call_cusparse(
    'gthr', x.dtype,
    handle, nnz, data_orig.data.ptr, x.data.data.ptr,
    P.data.ptr, cusparse.CUSPARSE_INDEX_BASE_ZERO)

It may have some time penalty compared to your change, but I suppose the consistency of memory usage is more important here. In nature, this problem comes from the different representations of a sparse matrix between cuSparse and CuPy. Three raw arrays for the former, and a wrapped-up class for the latter.

I see your point and agree that having hybrid in-place/copy doesn't seem like a good idea.

Copying the data to force in-place on x.data seems like a good way to not compromise performances too much. I'll do this change soon.

The other option would be to copy x.col and x.row and do nothing in-place, but this would be much worse impact on performances.

While pushing the changes I noticed that csrsort and cscsort also have this problematic pattern of reading and writing to the same buffer at the same time in gthr. See https://github.com/cupy/cupy/blob/master/cupy/cusparse.py#L534 and https://github.com/cupy/cupy/blob/master/cupy/cusparse.py#L562.
Also interestingly I wasn't able to create a test that would fail for csr or csc.
Do you know if there is something particular with csr and csc making it safe to call gthr that way? If not I'm happy to apply the fix there as well; but I'm not able to create a useful test for it.

pfnCI, test this please.

Successfully created a job for commit 8d2d4fa:

• Dashboard for commit 8d2d4fa

Thanks for the change, I kicked the CI again.

As for csrsort and cscsort, I know they have the same in-place gthr but didn't find they work safely. I'll look into it.

Jenkins CI test (for commit 8d2d4fa, target branch master) succeeded!

I got a test of csrsort that fails. Would you try the following?

@testing.with_requires('scipy')
class TestCsrsort(unittest.TestCase):

    def setUp(self):
        self.a = scipy.sparse.random(
            1, 10000, density=0.9, dtype=numpy.float32, format='csr')
        numpy.random.shuffle(self.a.indices)
        self.a.has_sorted_indices = False

    def test_csrsort(self):
        a = sparse.csr_matrix(self.a)
        cupy.cusparse.csrsort(a)

        self.a.sort_indices()
        testing.assert_array_equal(self.a.indptr, a.indptr)
        testing.assert_array_equal(self.a.indices, a.indices)
        testing.assert_array_almost_equal(self.a.data, a.data)

@takagi thanks for the useful test case. I added the fix on csrsort and cscsort as well, and added corresponding tests following your suggestion.

pfnCI, test this please.

Successfully created a job for commit f84345e:

• Dashboard for commit f84345e

Jenkins CI test (for commit f84345e, target branch master) succeeded!

LGTM, thanks!