pyt-team / TopoModelX

Topological Deep Learning
MIT License
219 stars 79 forks source link

Adding util method to directly convert sparse matrix to torch #233

Closed jkhouja closed 11 months ago

jkhouja commented 11 months ago

Here's a PR with proposed solution for https://github.com/pyt-team/TopoModelX/issues/209. I tried few variations and this seems to be the most efficient. It doesn't seem to affect speed on smaller dimensions as in here but definitely on a larger sparse matrix:

$test_matrix =  coo_matrix(([3,4,5,-3.2], ([0,1,1,200], [2,0,2,500])), shape=(2000,3000))

# current way
$ %timeit torch.from_numpy(test_matrix.todense()).to_sparse().to_dense()
5.63 ms ± 66.9 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

# with proposed util function
$ %timeit to_torch_coo(test_matrix)
24.2 µs ± 129 ns per loop (mean ± std. dev. of 7 runs, 10,000 loops each)

Since I'm still getting familiar with the repo, I'm not sure where's the best place for this function so would love some feedback.

codecov[bot] commented 11 months ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (e53a4f9) 96.36% compared to head (c99e88b) 96.37%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #233 +/- ## ========================================== + Coverage 96.36% 96.37% +0.01% ========================================== Files 55 56 +1 Lines 2036 2044 +8 ========================================== + Hits 1962 1970 +8 Misses 74 74 ``` | [Files](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team) | Coverage Δ | | |---|---|---| | [topomodelx/utils/sparse.py](https://app.codecov.io/gh/pyt-team/TopoModelX/pull/233?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pyt-team#diff-dG9wb21vZGVseC91dGlscy9zcGFyc2UucHk=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jkhouja commented 11 months ago

@mhajij @papamarkou once we agree on the utility function, I'll replace our current casting usage in the repo to use it.