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

Add edge betweenness centrality #799

Merged
merged 12 commits into from
Mar 9, 2023

Conversation

tapanih
Copy link
Contributor

@tapanih tapanih commented Feb 1, 2023

Related to #441

Added edge betweenness centrality to rustworkx and rustworkx-core. It is mostly based on betweenness centrality with some modifications.

@coveralls
Copy link

coveralls commented Feb 3, 2023

Pull Request Test Coverage Report for Build 4374721255

  • 192 of 192 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 97.145%

Totals Coverage Status
Change from base Build 4366049397: 0.04%
Covered Lines: 14053
Relevant Lines: 14466

💛 - Coveralls

@mtreinish mtreinish self-assigned this Feb 4, 2023
@jlapeyre
Copy link
Collaborator

jlapeyre commented Feb 6, 2023

Thanks for the contribution!
I think it would be a good idea to cross-reference (vertex) betweeness centrality somehow. networkx has quite a bit more of this. But I don't want to overload this PR. Also, rustworkx is less complete and has less need at present. networkx implements something like forty-two centralities. With this PR, rustworkx will have three.

But I think including something like a "See also" like this would be useful.

Comment on lines 23 to 24
For example, computing the edge betweenness centrality for all edges in a 5x5
grid graph and using that to color the edges in a graph visualization:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
For example, computing the edge betweenness centrality for all edges in a 5x5
grid graph and using that to color the edges in a graph visualization:
For example, the following computes the edge betweenness centrality for all edges in a 5x5
grid graph and uses the result to color the edges in a graph visualization:

- |
Added a new function to rustworkx-core ``edge_betweenness_centrality`` to
the ``rustworkx_core:centrality`` module which computes the edge betweenness
centrality of all edges in a given graph.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • There are several lines above that consist only of whitespace. This should be removed (only a single newline for each such line.)
  • There should be a newline at the end of the file.

- |
Added a new function to rustworkx-core ``edge_betweenness_centrality`` to
the ``rustworkx_core:centrality`` module which computes the edge betweenness
centrality of all edges in a given graph.
Copy link
Collaborator

Choose a reason for hiding this comment

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

File needs a newline.

@@ -326,3 +326,119 @@ pub fn digraph_eigenvector_centrality(
))),
}
}

/// Compute the edge betweenness centrality of all edges in a :class:`~PyGraph`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The edge and vertex centrality functions should be next to each other in this file, as they are more closely related to each other than to the eigenvector centrality... unless there is another good reason for the current order.

Copy link
Collaborator

@jlapeyre jlapeyre left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks for pushing this! (and sorrt for the delay in final review)

@mtreinish mtreinish added the automerge Queue a approved PR for merging label Mar 9, 2023
@mergify mergify bot merged commit 004dc16 into Qiskit:main Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants