Implementation of closeness_centrality #593
Conversation
|
|
There was a problem hiding this comment.
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.
| 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.
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.
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.
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.
Pull Request Test Coverage Report for Build 4387478694
💛 - Coveralls |
This PR implements closeness centrality for graphs and digraphs.
It supports boolean option
wf_improvedwith the same names and meanings as their networkx counterparts.#441