-
Notifications
You must be signed in to change notification settings - Fork 160
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 eigenvector centrality function #621
Conversation
This commit adds a new function eigenvector_centrality() to compute the eigenvector centrality of a graph. It uses the same power function approach that the NetworkX function eigenvector_centrality() [1] function uses. This is for two reasons, the first is that a more traditional eigenvector linear algebra/BLAS function would either require us to link against BLAS at build time (which is a big change in the build system and a large requirement) or to call out to numpy via python both of which seemed less than ideal. The second reason was to make handling holes in node indices bit easier. Using this approach also enabled us to put the implementation in retworkx-core so it can be reused with any petgraph graph. Part of Qiskit#441
Pull Request Test Coverage Report for Build 2477501415
💛 - Coveralls |
src/centrality.rs
Outdated
let cost_fn = CostFn::try_from((weight_fn, default_weight))?; | ||
let ev_centrality = centrality::eigenvector_centrality( | ||
&graph.graph, | ||
|e| cost_fn.call(py, e.weight()), |
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.
It might be beneficial to precompute and store all edge weights like we do in all_pairs_dijkstra_path_lengths
so we avoid calling Python overhead each iteration.
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.
Let me benchmark this to see if it makes a difference (we only did the precomputation in the all pairs functions because it operates in parallel and we didn't want to be gil bound in parallel threads)
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.
I benchmarked this and it's basically the same but marginally faster in a couple of cases:
not really a noticeable difference. But I think I'll make the change anyway, mostly because it does open up potential parallelization. I think I can make the inner loop execute in parallel and will test that in bit. (I played with this a bit and I don't think it's actually feasible without locking which would defeat the purpose of doing it in parallel)
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.
Oh, actually ignore that graph I forgot to set a weight callback function in the benchmark so that's improvement is just the time for using default weight. Which still seems worthwhile to me
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.
We might want to benchmark against a more "difficult" graph since a path graph will converge after 12 iterations (I think) so the performance difference will be really minor.
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.
I pushed this up in: 3204e09 I'm still waiting on the data collection for the run with a python callback. It's been running the sweep for ~60min and the precomputed weights sweep took ~15min. So there is no doubt this is faster. I'll generate the graph when it eventually finishes just for posterity.
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.
Co-authored-by: georgios-ts <[email protected]>
Co-authored-by: georgios-ts <[email protected]>
Co-authored-by: georgios-ts <[email protected]>
This commit fixes an issue where we were overcounting parallel edges in the graph. There was a bug in the logic that was calculating the combined weight of parallel edges incorrectly. Co-authored-by: georgios-ts <[email protected]>
This commit changes the callback from the retworkx functions for determining edge weight to precompute them to avoid requiring a python context in the callback. This make a minor improvement in runtime since we only callback to python once at the very beginning instead of in the middle of the loop.
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.
Thanks for all the updates. Some minor comments but it's good to go.
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.
Oh, we'll need to update the docs before we merge.
Co-authored-by: georgios-ts <[email protected]>
Co-authored-by: georgios-ts <[email protected]>
This commit adds a new function eigenvector_centrality() to compute the
eigenvector centrality of a graph. It uses the same power function
approach that the NetworkX function eigenvector_centrality() [1]
function uses. This is for two reasons, the first is that a more
traditional eigenvector linear algebra/BLAS function would either
require us to link against BLAS at build time (which is a big change
in the build system and a large requirement) or to call out to numpy
via python both of which seemed less than ideal. The second reason was
to make handling holes in node indices bit easier. Using this approach
also enabled us to put the implementation in retworkx-core so it can be
reused with any petgraph graph.
Part of #441