Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax condition on square shape on sparse input #48

Closed
ulupo opened this issue Sep 9, 2020 · 1 comment · Fixed by #55
Closed

Relax condition on square shape on sparse input #48

ulupo opened this issue Sep 9, 2020 · 1 comment · Fixed by #55

Comments

@ulupo
Copy link
Collaborator

ulupo commented Sep 9, 2020

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

vertices = np.ones(adjacency_matrix.shape[0], dtype=np.float)
or
vertices = np.asarray(adjacency_matrix.diagonal())
.

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
Copy link
Collaborator Author

ulupo commented Dec 3, 2020

Fixed by #55

@ulupo ulupo closed this as completed Dec 3, 2020
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 a pull request may close this issue.

1 participant