Degree centrality implementation #1306
Conversation
|
ons seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
ons seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Thanks for making your first contribution. I left some comments, but some things to note:
- Add release notes: https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#release-notes
- Add a universal function so your code works with both
PyGraphandPyDiGraph. Here is a good example:rustworkx/rustworkx/__init__.py
Line 1273 in 8774ccd
- You probably want to implement this centrality for
PyDiGraphas well - Format your code. It is an automated process, see https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md#style
Also, I noticed from the CLA bot that this PR has two authors. This is quite unusual for a first contribution so please let me know if you are:
- Working on a college assignment together
- One of the users flagged by the bot is an AI Assistant
There is nothing wrong with either, but it can help me review the PR accordingly.
tests/graph/test_centrality.py
Outdated
| @@ -230,3 +230,46 @@ def test_custom_graph_unnormalized(self): | |||
| expected = {0: 9, 1: 9, 2: 12, 3: 15, 4: 11, 5: 14, 6: 10, 7: 13, 8: 9, 9: 9} | |||
| for k, v in centrality.items(): | |||
| self.assertAlmostEqual(v, expected[k]) | |||
|
|
|||
|
|
|||
| class TestDegreeCentrality(unittest.TestCase): | |||
There was a problem hiding this comment.
The existing test coverage is good, but remember to add a test with the multigraph case to check that we cover it properly
|
Also, we can handle |
|
Hi, thank you for the swift feedback! I just wanted to note that this was in fact a college assignment, with the other contributor being my classmate. However we are now moving on to individual work, so I will be responsible for the next steps. I will be begin addressing your suggestions shortly. |
Sounds good to me, just remember to ask your classmate(s) to sign the CLA otherwise the bot will not let it get merged |
| /// } | ||
| /// ``` | ||
|
|
||
| pub fn graph_degree_centrality<G>(graph: G) -> Vec<f64> |
There was a problem hiding this comment.
For rustworkx-core, use a generic method that takes both kind of graphs.
rustworkx/rustworkx-core/src/centrality.rs
Line 632 in aea56cf
| } | ||
| self.assertEqual(expected, centrality) | ||
|
|
||
| def test_digraph_degree_centrality_with_direction(self): |
There was a problem hiding this comment.
Minor organization detail. This should put the digraph tests in tests/digraph/test_centrality.py with the other directed graph tests
Proposed a solution for #1129. The implementation is straightforward, though I wonder whether I should add a flag to support in-degree and out-degree centrality, and support for weighted graphs (since the current implementation assumes an unweighted graph). Let me know what you think!