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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] BUG Update weights check in bc and graph prims wrappers #1290

Merged
merged 6 commits into from
Dec 4, 2020
Merged

[REVIEW] BUG Update weights check in bc and graph prims wrappers #1290

merged 6 commits into from
Dec 4, 2020

Conversation

hlinsen
Copy link
Contributor

@hlinsen hlinsen commented Dec 1, 2020

This PR fixes the check for weights in betweenness_centrality and edge_betweenness_centrality wrappers.
Addresses issue #1282.

@hlinsen hlinsen requested a review from a team as a code owner December 1, 2020 19:07
@GPUtester
Copy link
Contributor

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@BradReesWork BradReesWork added this to the 0.18 milestone Dec 1, 2020
@BradReesWork BradReesWork added non-breaking Non-breaking change bug Something isn't working labels Dec 1, 2020
@BradReesWork BradReesWork changed the base branch from branch-0.18 to branch-0.17 December 1, 2020 19:14
@BradReesWork BradReesWork modified the milestones: 0.18, 0.17 Dec 1, 2020
@BradReesWork BradReesWork linked an issue Dec 1, 2020 that may be closed by this pull request
[BUG] Betweenness Centrality crashes when there are edgge weights #1282
Closed
@hlinsen hlinsen requested review from a team as code owners December 1, 2020 19:16
@raydouglass raydouglass removed the request for review from a team December 1, 2020 20:53
afender
afender requested changes Dec 1, 2020
View reviewed changes
Copy link
Member

@afender afender left a comment

Choose a reason for hiding this comment

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

Thanks, @hlinsen. Could you elaborate on this fix in the context of my earlier comment:
#1282 (comment)

In particular, I think we should throw an error when weights are passed since this is not supported.

BradReesWork
BradReesWork requested changes Dec 1, 2020
View reviewed changes
Copy link
Member

@BradReesWork BradReesWork left a comment

Choose a reason for hiding this comment

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

there should be an else condition after the weight checks

@hlinsen
Copy link
Contributor Author

hlinsen commented Dec 1, 2020 •
edited
Loading

Thanks, @hlinsen. Could you elaborate on this fix in the context of my earlier comment:
#1282 (comment)

In particular, I think we should throw an error when weights are passed since this is not supported.

The weight parameter option is currently throwing an error here: https://github.com/rapidsai/cugraph/blob/branch-0.17/python/cugraph/centrality/betweenness_centrality.py#L237. The ones inside the wrapper are used to construct the graph but are ignored during computation.

@codecov-io
Copy link

codecov-io commented Dec 2, 2020 •
edited
Loading

Codecov Report

Merging #1290 (c537ca5) into branch-0.17 (6b92349) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           branch-0.17    #1290   +/-   ##
============================================
  Coverage        60.38%   60.38%           
============================================
  Files               67       67           
  Lines             3029     3029           
============================================
  Hits              1829     1829           
  Misses            1200     1200

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b92349...c537ca5. Read the comment docs.

afender
afender approved these changes Dec 2, 2020
View reviewed changes
Copy link
Member

@afender afender 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 adding the test and clarifying @hlinsen 👍

@hlinsen
Copy link
Contributor Author

hlinsen commented Dec 2, 2020

rerun tests

@BradReesWork
Copy link
Member

rerun tests

2 similar comments
@hlinsen
Copy link
Contributor Author

hlinsen commented Dec 3, 2020

rerun tests

@hlinsen
Copy link
Contributor Author

hlinsen commented Dec 4, 2020

rerun tests

@BradReesWork
Copy link
Member

rerun tests

1 similar comment
@hlinsen
Copy link
Contributor Author

hlinsen commented Dec 4, 2020

rerun tests

@BradReesWork BradReesWork merged commit f45c221 into rapidsai:branch-0.17 Dec 4, 2020
@hlinsen hlinsen deleted the bc-fix branch April 15, 2021 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afender afender afender approved these changes

@BradReesWork BradReesWork BradReesWork approved these changes

Assignees
No one assigned
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Milestone
0.17
Development

Successfully merging this pull request may close these issues.

[BUG] Betweenness Centrality crashes when there are edgge weights
5 participants
@hlinsen @GPUtester @codecov-io @BradReesWork @afender