Skip to content
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

Simplifying betweenness_centrality (for vertices) #815

Merged
merged 7 commits into from
Mar 10, 2023

Conversation

jlapeyre
Copy link
Collaborator

@jlapeyre jlapeyre commented Feb 10, 2023

#799 "Add edge betweenness centrality" spurred me to try to reduce code repetition and complexity in betweenness centrality (for vertices). I recall trying rayon-cond in the past. In any case, it seems to work here now.

@coveralls
Copy link

coveralls commented Feb 10, 2023

Pull Request Test Coverage Report for Build 4388571347

  • 36 of 36 (100.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.04%) to 97.136%

Files with Coverage Reduction New Missed Lines %
src/shortest_path/all_pairs_dijkstra.rs 1 98.54%
Totals Coverage Status
Change from base Build 4387668543: 0.04%
Covered Lines: 14039
Relevant Lines: 14453

💛 - Coveralls

@jlapeyre jlapeyre changed the title [WIP] Simplifying betweenness_centrality (for vertices) Simplifying betweenness_centrality (for vertices) Feb 11, 2023
@jlapeyre jlapeyre marked this pull request as ready for review February 11, 2023 21:38
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for reviving your old PR. I always felt bad that it fell through the cracks so it's good to get this simplification merged now.

@@ -75,7 +74,7 @@ use rayon_cond::CondIterator;
/// [`edge_betweenness_centrality`]
pub fn betweenness_centrality<G>(
graph: G,
endpoints: bool,
include_endpoints: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this ok from a backwards compatibility perspective since in rust the arguments are all passed positionally (this would be breaking in Python so I wanted to just note this and check myself).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that's a good point.

@mtreinish mtreinish added the automerge Queue a approved PR for merging label Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Queue a approved PR for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants