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

Add parallel version of edge_betweenness_centrality #60

Conversation

jkrajniak
Copy link
Contributor

Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

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

Thanks @jkrajniak!

I've left a few comments below. LMK what u think.
And there are a few linting errors in this PR which you will be able to see once a maintainer approves running the wf test. you can run black . to get rid of most of them.

benchmarks/benchmarks/bench_centrality.py Outdated Show resolved Hide resolved
@dschult dschult added the type: Enhancement New feature or request label Apr 29, 2024
Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @jkrajniak!

I've added a few comments below. Thanks!

timing/timing_all_functions.py Outdated Show resolved Hide resolved
nx_parallel/algorithms/centrality/betweenness.py Outdated Show resolved Hide resolved
@dschult
Copy link
Member

dschult commented May 28, 2024

Looks like all tests pass now. @Schefflera-Arboricola can you check your comments as "resolved" -- or leave more comments, or another review. When it's ready, select the "Approve" button while leaving a review.
Thanks for this PR @jkrajniak

@Schefflera-Arboricola
Copy link
Member

can you check your comments as "resolved"

no, I think only the author or maintainers can do that.

@jkrajniak
Copy link
Contributor Author

can you check your comments as "resolved"

no, I think only the author or maintainers can do that.

You can just click on the "Resolve conversation" button for each of the comments.

@dschult
Copy link
Member

dschult commented May 31, 2024

Unless there is no button that says "Resolve conversation". :} I think github only allows resolving conversations for maintainers and the original poster. We are working on getting maintainer status for @Schefflera-Arboricola -- proper channels and all that. Meanwhile I resolved those that I think have been addressed. I wasn't sure about one where ruff made changes so it should be looked at. The ruff changes are usually "linting" changes, e.g. formatting, whitespace and indentation/line wraps changes. So it's probably OK.

for i in ebc:
nodes_ebc[i[0]] += ebc[i]
nodes_ebc[i[1]] += ebc[i]
def get_chunk_function(ebc):

Choose a reason for hiding this comment

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

I don't think there is any need for 2 functions here(i.e. get_chunk_function and get_chunk) and ebc doesn't need to be passed as it is already in the outer function's namespace. LMK what you think. Thanks!

@Schefflera-Arboricola
Copy link
Member

Meanwhile I resolved those that I think have been addressed.

Thanks!

I wasn't sure about one where ruff made changes so it should be looked at. The ruff changes are usually "linting" changes, e.g. formatting, whitespace and indentation/line wraps changes. So it's probably OK.

There were no style changes(or any changes) in that file. I'm not sure what @jkrajniak was referring to there.

@dschult
Copy link
Member

dschult commented Jun 1, 2024

I am assuming that we don't get to see the changes. They were probably made by pre-commit locally before pushed to the repo -- so we would not see them. It all looks good from my perspective too, so I will resolve that comment. Thanks!

README.md Outdated Show resolved Hide resolved
@jkrajniak jkrajniak force-pushed the feature/edge_betweenness_centrality branch from f2c4950 to e0d937f Compare June 9, 2024 19:54
Copy link
Member

@Schefflera-Arboricola Schefflera-Arboricola left a comment

Choose a reason for hiding this comment

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

Thank you @jkrajniak :)

Copy link
Member

@dschult dschult left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@Schefflera-Arboricola Schefflera-Arboricola merged commit 16ae504 into networkx:main Jun 13, 2024
11 checks passed
@jarrodmillman jarrodmillman added this to the 0.3 milestone Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: Enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

4 participants