Improve dot performance

[daletovar]

• improves dot for two COO arrays, addresses Performance bencmarks comparison with scipy.sparse #331

  • I used the same algorithm that scipy uses.
  • It's a bit slower than scipy because to ensure sorted coordinates, we have to do some sorting.

• add dot for GCXS arrays

  • Uses the same algorithm.
  • Works for csr @ csr and csc @ csc
  • I found that the csr @ csc dot product-based algorithm was just to slow, so we convert one of the arrays.

• add io for GCXS arrays

[hameerabbasi]

LGTM caught some valid concerns -- Could you address those?

[hameerabbasi]

And the code coverage is a bit lacking. 😕

[daletovar]

Thanks @rgommers, I'll make sure to do that going forward.

[daletovar]

With regards to the sorting, the issue is that the dot algorithm produces results that are not sorted:

In [1]: import scipy.sparse as ss
In [2]: x = ss.random(1000,1000,density=.2,format='csr')
In [3]: y = ss.random(1000,1000,density=.2,format='csr')
In [4]: res = x @ y
In [5]: res.indptr[1]
Out[5]: 1000
In [6]: res.indices
Out[6]: array([554, 328, 723, ...,  14,   7,   6], dtype=int32)

The indices for each row of the resulting matrix are out-of-order. So I added something like this:

for start, stop in zip(res.indptr[:-1], res.indptr[1:]):
    order = np.argsort(res.indices[start:stop])
    res.data[start:stop] = res.data[start:stop][order]
    res.indices[start:stop] = res.indices[start:stop][order]

[daletovar]

@hameerabbasi, yes I'll see if I can fix those.

[lgtm-com]

This pull request introduces 2 alerts and fixes 1 when merging 5b4f08a into e682a8b - view on LGTM.com

new alerts:

• 1 for Unused local variable
• 1 for Unused import

fixed alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging eb8c0e5 into e682a8b - view on LGTM.com

new alerts:

• 1 for Unused import

fixed alerts:

• 1 for Unused import

[lgtm-com]

This pull request introduces 1 alert and fixes 1 when merging ae63040 into e682a8b - view on LGTM.com

new alerts:

• 1 for Unused import

fixed alerts:

• 1 for Unused import

[daletovar]

Here's an initial mark:

In [2]: x = sparse.random((5000, 5000), density=0.001)
In [3]: y = sparse.random((5000, 5000), density=0.001)
In [4]: xs = x.tocsr()
In [5]: ys = y.tocsr()
In [6]: x @ y # jit
Out[6]: <COO: shape=(5000, 5000), dtype=float64, nnz=124139, fill_value=0.0>
In [7]: %timeit x @ y
10.5 ms ± 104 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In [8]: %timeit xs @ ys
1.14 ms ± 2.9 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

[hameerabbasi]

Can we possibly sort before putting in the data? I suspect it'll be a lot faster.

[daletovar]

I'm not quite sure what you mean. Could you give an example?

[hameerabbasi]

Well, the original CSR-CSR multiplication algorithm doesn't require sorting, correct? I suspect this one only requires it because you're passing in unsorted data in the first place (or iterating over it in a way that's unsorted).

Could you possibly pre-sort the data so the output here will be sorted?

[daletovar]

The original CSR-CSR multiplication algorithm iterates over the data in a way that's unsorted. In scipy, the resulting csr_matrix has indices that are out-of-order:

In [13]: xs.has_sorted_indices
Out[13]: 1
In [14]: res = xs @ ys
In [15]: res.has_sorted_indices
Out[15]: 0

Since many of the operations for COO and GCXS rely on maintaining sorted coordinates, I was thinking it would be best to ensure that the result of dot has sorted coords.

[hameerabbasi]

In that case could you post a benchmark which includes sorting for the SciPy version?

[daletovar]

%%timeit
x @ y # coo coo
10.2 ms ± 51.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

%%timeit
res = xs @ ys
res.sort_indices()
4.05 ms ± 8.83 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

I'm going to test doing just one sort at the end to see how that does.

[daletovar]

%%timeit
x @ y # coo coo - one sort at the end
7.55 ms ± 96.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

%%timeit
res = xs @ ys
res.sort_indices()
4.04 ms ± 15.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

It looks like doing one sort at the end is better.

[hameerabbasi]

Could you also post some GCXS-GCXS benchmarks for direct comparison with SciPy?

[daletovar]

%%timeit
x @ y # csr csr
6.32 ms ± 107 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

x = x.change_compressed_axes([1])
y = y.change_compressed_axes([1])
x @ y

%%timeit
x @ y # csc csc
6.11 ms ± 46.5 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

x = x.tocoo()
y = y.tocoo()
x @ y

%%timeit
x @ y # coo coo
7.23 ms ± 93.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

%%timeit
res = xs @ ys
res.sort_indices()
4.08 ms ± 49.1 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

[hameerabbasi]

This was a compat check for Python 2. It can be removed.

[hameerabbasi]

Can you rewrite this using tmp_path: https://docs.pytest.org/en/latest/tmpdir.html#the-tmp-path-fixture

[hameerabbasi]

Same for the next test.

[hameerabbasi]

A few changes. Looks pretty awesome overall!

[daletovar]

Thanks so much for the review, @hameerabbasi. I think those were really helpful changes. Is there anything else you'd like to see?

[hameerabbasi]

This should be added back, only the "if" was unnecessary.

[hameerabbasi]

I have one final question: Does dot now return GCXS by default for COO/ndarray inputs? If so, we should change this.

And one possibility for future work: Writing a sparse-dense matrix multiplication kernel separately.

[daletovar]

No, the default behavior is the same. I did make it so that GCXS @ COO results in a GCXS though.

And one possibility for future work: Writing a sparse-dense matrix multiplication kernel separately.

+1

I'm away from my computer the next couple of days so I apologize if I don't get back right away about anymore suggestions.

[hameerabbasi]

Thanks, @daletovar!