Add parallelism to cloness_centrality#1392
Conversation
Pull Request Test Coverage Report for Build 14346065276Details
💛 - Coveralls |
|
Because #1387, I added a test to boost the coverage in the report. I am confident the Rust tests guarantee the correctness so for now that is a stop gap |
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
This LGTM, it makes a lot of sense to run this with rayon. Thanks for doing this. I just left a couple of comments inline the main one is I think we can do this without a mutex using a rayon iterator collection into the vec which should be faster..
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
Overall this looks much better to me now, thanks for making the change. I just had a couple of small inline comments about the release notes and the docstring updates. But code-wise I think this is good to go.
releasenotes/notes/parallel-closeness-centrality-5165f62e370181eb.yaml
Outdated
Show resolved
Hide resolved
mtreinish
left a comment
mtreinish
left a comment
There was a problem hiding this comment.
I think this looks fine now, besides the lint failure. The RAYON_NUM_THREADS doesn't look like it's in the Python docs though, we normally document it there too because it's an env variable and works from python too.
|
Let me update the Python docs and fix the formatting |
* First look at parallel implementation * Add parallel threshold parameter * Add parllel_threshold to Python arguments * Make weighted closeness centrality also parallel * Add new argument to type stubs * Document parameters for newman's type specific functions * Add parallel tests to Python * Add release notes for the change * Boost coverage of Newman Weighted Centrality via unweighted test case * Simplify implementation with .collect() * Add parallel_threshold to docstring * Fix cargo clippy warnings * Document RAYON_NUM_THREADS * Move reno section to upgrade * Add rayon num threads detail to Python * Update Python docstring as well * Try to fix indentation
3 participants
This PR makes
closeness_centralityandnewman_weighted_closeness_centralityparallel for better performance. This is possible because the task is embarrassingly parallel.We probably missed this in #593 but #1385 reminded me of it.
The parallelism is toggled via the
parallel_thresholdjust like other centrality methods.