Skip to content

feat: implementation for group_degree_centrality, group_closeness_centrality, and group_betweenness_centrality#1565

Merged
IvanIsCoding merged 15 commits intoQiskit:mainfrom
selmanozleyen:feat/group-centrality-wip
Apr 15, 2026
Merged

feat: implementation for group_degree_centrality, group_closeness_centrality, and group_betweenness_centrality#1565
IvanIsCoding merged 15 commits intoQiskit:mainfrom
selmanozleyen:feat/group-centrality-wip

Conversation

@selmanozleyen
Copy link
Copy Markdown
Contributor

@selmanozleyen selmanozleyen commented Mar 12, 2026

hi I added these three functions because I'd like to use them in squidpy instead of pure python version.

  • Add group_degree_centrality, group_closeness_centrality, and
    group_betweenness_centrality for both digraph and graph

Note

While writing cross-validation tests against NetworkX, I discovered that
NetworkX's group_betweenness_centrality is buggy on directed graphs that
are not strongly connected (returns negative values, crashes with KeyError).
Filed upstream: networkx/networkx#8559.
The NX comparison tests here only use strongly-connected directed graphs to
avoid this.

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Mar 12, 2026

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@selmanozleyen selmanozleyen changed the title Feat/group centrality wip feat: implementation for group_degree_centrality, group_closeness_centrality, and group_betweenness_centrality Mar 12, 2026
@selmanozleyen selmanozleyen marked this pull request as ready for review March 12, 2026 02:32
@selmanozleyen selmanozleyen marked this pull request as draft March 12, 2026 02:32
@selmanozleyen selmanozleyen marked this pull request as ready for review March 12, 2026 11:20
@IvanIsCoding
Copy link
Copy Markdown
Collaborator

Some preliminary thoughts:

  1. Fix clippy lints, naturally
  2. I'd decouple the tests from NetworkX. If you can just put the expected centrality in a dictionary that is fine. Naturally, document it with #obtained with: g_nx = nx.cycle_graph(6, create_using=nx.DiGraph) etc. so we can reproduce the numbers

Using NetworkX as a ground source of truth is a great idea. But it's also nice not to be affected by their API changes. We had to do that for other centrality methods.

@selmanozleyen
Copy link
Copy Markdown
Contributor Author

thanks for the feedback! I made the lints + hardcoded the tests while documenting the nx version to get them.

I undid the unrelated formatting/cleanup to this PR here: 7904d20

I can revert it if you want to include them

@IvanIsCoding
Copy link
Copy Markdown
Collaborator

IvanIsCoding commented Apr 2, 2026

We changed the Python formatter so I guess the failures are on me. Oh well

just check https://github.com/Qiskit/rustworkx/blob/main/CONTRIBUTING.md, make nox -elint pass locally, add a release note. And then I can intervene and wrap it up before merging.

0.18 has been delayed regardless, so your PR will still make the release.

@IvanIsCoding IvanIsCoding added this to the 0.18.0 milestone Apr 2, 2026
@selmanozleyen
Copy link
Copy Markdown
Contributor Author

ok thanks I added the release note plus the lints

@IvanIsCoding
Copy link
Copy Markdown
Collaborator

I pushed a minor change to compute the length from the HashSet, but that is it. This should be good to go.

@IvanIsCoding
Copy link
Copy Markdown
Collaborator

group_betweenness_centrality was easy to parallelize, so I profited to do that as well. I will merge your PR and it will make it to the 0.18.0 release, whenever that happens.

@IvanIsCoding IvanIsCoding enabled auto-merge April 15, 2026 04:17
@IvanIsCoding IvanIsCoding added this pull request to the merge queue Apr 15, 2026
Merged via the queue into Qiskit:main with commit d0ec087 Apr 15, 2026
38 checks passed
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.

3 participants