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

[Misc] Provide a "return 1s instead of edge IDs" option in scipy adjacency matrix #730

Merged
merged 6 commits into from
Jul 29, 2019

Conversation

BarclayII
Copy link
Collaborator

Description

Added a return_edge_id optional argument in DGLGraph.adjacency_matrix_scipy() method which defaults to True. If False, then 1's are returned.

The reason to do this is that if edge IDs are returned, then one of the nonzero entries will be actually 0, which may be confusing.

In the next major release (0.5) it will default to False to keep consistent with adjacency_matrix interface. Since existing code may depend on the current behavior that returns edge IDs, I added a FutureWarning.

This PR is separated from #720

Checklist

Please feel free to remove inapplicable items for your PR.

  • The PR title starts with [$CATEGORY] (such as [Model], [Doc], [Feature]])
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage
  • Code is well-documented
  • To the my best knowledge, examples are either not affected by this change,
    or have been fixed to be compatible with this change

@BarclayII BarclayII requested a review from yzh119 July 29, 2019 09:25
Copy link
Member

@yzh119 yzh119 left a comment

Choose a reason for hiding this comment

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

It seems you haven't changed the elements in the returned matrix accordingly.

python/dgl/graph_index.py Outdated Show resolved Hide resolved
rst = _CAPI_DGLGraphGetAdj(self, transpose, fmt)
if fmt == "csr":
indptr = utils.toindex(rst(0)).tonumpy()
indices = utils.toindex(rst(1)).tonumpy()
shuffle = utils.toindex(rst(2)).tonumpy()
shuffle = utils.toindex(rst(2)).tonumpy() if return_edge_ids else np.ones_like(indices)
Copy link
Member

Choose a reason for hiding this comment

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

I recommend to rename this variable as data.

n = self.number_of_nodes()
return scipy.sparse.csr_matrix((shuffle, indices, indptr), shape=(n, n))
elif fmt == 'coo':
idx = utils.toindex(rst(0)).tonumpy()
n = self.number_of_nodes()
m = self.number_of_edges()
row, col = np.reshape(idx, (2, m))
shuffle = np.arange(0, m)
shuffle = np.arange(0, m) if return_edge_ids else np.ones_like(row)
Copy link
Member

Choose a reason for hiding this comment

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

The same.

@yzh119 yzh119 merged commit 86f28d6 into dmlc:master Jul 29, 2019
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.

2 participants