Skip to content

Conversation

@eriknw
Copy link
Member

@eriknw eriknw commented Feb 18, 2023

Closes #384.

This uses fast_matrix_market (which looks pretty fast) by default if available, but will fall back to scipy if necessary. scipy is still necessary to be installed even if using "fast_matrix_market" engine.

There were three small issues I encountered that I plan to raise upstream, but I was able to work around them.

TODO:

@coveralls
Copy link

coveralls commented Feb 18, 2023

Coverage Status

Changes unknown
when pulling d9ceebc on eriknw:fast-matrix-market
into ** on python-graphblas:main**.

@eriknw
Copy link
Member Author

eriknw commented Feb 19, 2023

CC , @alugowski fast-matrix-market works pretty well as a near-drop-in replacement for scipy.io.mmread, and I'm really happy to improve the user experience around MM files--thanks! I had to work around a few tiny differences, which wasn't that difficult, so these aren't that important:

@alugowski, when writing Matrix Market files, do you recommend passing scipy CSR or CSC matrices instead of COO? I'm curious whether fmm or SuiteSparse:GraphBLAS would handle this faster.

Comment on lines 553 to 555
if _output_type(A) is _Vector:
indices, values = A.to_coo(sort=False)
s = COO(indices, values, shape=A.shape)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw @SultanOrazbayev I just special-cased Vector in to_pydata_sparse, b/c previously it was creating a (1, n)-shaped sparse array from a Vector instead of a (n,)-shaped array.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, sorry about that!

Copy link
Member

@SultanOrazbayev SultanOrazbayev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, @eriknw !

Comment on lines +671 to +677
try:
from fast_matrix_market import mmwrite # noqa: F811
except ImportError: # pragma: no cover (import)
if engine != "auto":
raise ImportError(
"fast_matrix_market is required to write Matrix Market files "
f'using the "{engine}" engine'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure to what extent DRY principle is useful here, but perhaps there is scope for generalising this check for suitable import as some utility function (as a separate PR).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to such a PR :)

You may notice that my style sometimes doesn't adhere to DRY for small, straightforward (to me) things, but I suspect there are cases where things could be improved.

@eriknw
Copy link
Member Author

eriknw commented Mar 21, 2023

Thanks for reviewing @SultanOrazbayev!

I just finished the final TODO for this PR--I added documentation to io.rst.

@eriknw eriknw merged commit 4920dc8 into python-graphblas:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Option to read MatrixMarket with fast_matrix_market

3 participants