search

giotto-ai / pyflagser

Python bindings and API for the flagser C++ library (https://github.com/luetge/flagser).
Other
13 stars 14 forks source link

Relax condition on square shape on sparse input #48

Closed ulupo closed 3 years ago

ulupo commented 3 years ago

Description

It seems to me that it is unnecessarily unfriendly for the user dealing with sparse matrices to impose that these must be square.

Steps/Code to Reproduce

The example of a directed 2-simplex already displays this. Since this is a triangle with vertex 0 -> 1, 1 -> 2, and 0 -> 2, the user might decide to represent this as:

from scipy.sparse import coo_matrix

data = np.ones(3)
row = np.array([0, 1, 0])
col = np.array([1, 2, 2])
ad = coo_matrix((data, (row, col)))

ad has shape (2, 3) when constructed this way. When passing this input to either _extract_unweighted_graph or _extract_weighted_graph, the user first gets a warning, and then ultimately an error from the C++ code like this:

RuntimeError: Out of bounds, tried to add an edge involving vertex 2, but there are only 2 vertices.

This is because the number of vertices is guessed to be adjacency_matrix.shape[0] in https://github.com/giotto-ai/pyflagser/blob/7e5d09a34b763279b0c4ed80515c3e44ba1ce589/pyflagser/_utils.py#L13 or https://github.com/giotto-ai/pyflagser/blob/7e5d09a34b763279b0c4ed80515c3e44ba1ce589/pyflagser/_utils.py#L43.

Proposal

I suggest that we estimate the number of vertices as max(adjacency_matrix.shape) instead of the size of axis 0. I also propose that we remove the square warning or only keep it for dense input.

ulupo commented 3 years ago

Fixed by #55