-
Notifications
You must be signed in to change notification settings - Fork 145
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
Implementation of closeness_centrality #593
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM, thanks for pushing this PR. I have a few inline comments including a suggestion on how to fix the MSRV job failure.
The only other thing is while you're updating this if you could also add a release note using reno
so we can document this new feature as part of 0.12.0. The procedure for doing this is documented here: https://github.com/Qiskit/retworkx/blob/main/CONTRIBUTING.md#release-notes
Pull Request Test Coverage Report for Build 2281936091
💛 - Coveralls |
@mtreinish |
tests/graph/test_centrality.py
Outdated
betweenness = retworkx.graph_closeness_centrality(self.graph) | ||
expected = {0: 0.5, 1: 0.75, 2: 0.75, 4: 0.5} | ||
self.assertEqual(expected, betweenness) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
betweenness = retworkx.graph_closeness_centrality(self.graph) | |
expected = {0: 0.5, 1: 0.75, 2: 0.75, 4: 0.5} | |
self.assertEqual(expected, betweenness) | |
closeness = retworkx.graph_closeness_centrality(self.graph) | |
expected = {0: 0.5, 1: 0.75, 2: 0.75, 4: 0.5} | |
self.assertEqual(expected, closeness) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks good to me. The code seems fine to me. I only have one concern about the docstrings specifically how they seem to be copied directly from networkx. If we're going to borrow the docstrings from there that's fine but we need to add a comment to the source to document the original authors of the text (everywhere we use it).
where :math:`d(v, u)` is the shortest-path distance between :math:`v` and | ||
:math:`u`, and :math:`n` is the number of nodes that can reach :math:`u`. | ||
|
||
Wasserman and Faust propose an improved formula for graphs with more than |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a link to the paper for this? If so it'd be good to include a link to it. (also in the docstrings)
Wasserman and Faust propose an improved formula for graphs with more than | ||
one connected component. The result is "a ratio of the fraction of actors | ||
in the group who are reachable, to the average distance" from the reachable | ||
actors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph looks identical to what's in the NetworkX documentation for this option: https://networkx.org/documentation/stable/reference/algorithms/generated/networkx.algorithms.centrality.closeness_centrality.html
It might be better to put this in your own words instead of copying exactly what NetworkX has in the documentation or provide a link to the original text as a comment to cite the original source of this text (same with the docstrings).
This merge commit fixes a number of merge conflicts caused by the project rename from retworkx to rustworkx and other changes since this was first opened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM now, thanks for pushing this. I went ahead and just rebased this on the current state of the main
branch and updated all the docstrings to put them in our own words and add a citation to the Wasserman & Faust paper.
Pull Request Test Coverage Report for Build 4387478694
💛 - Coveralls |
This PR implements closeness centrality for graphs and digraphs.
It supports boolean option
wf_improved
with the same names and meanings as their networkx counterparts.#441