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

Fix primitive bug discovered in MG edge betweenness centrality testing #3723

Merged

Conversation

ChuckHastings
Copy link
Collaborator

@jnke2016 found a bug in MG edge betweenness centrality. Upon investigation, the major_offset in this block of code wasn't being set correctly if major is in the hypersparse region.

@ChuckHastings ChuckHastings self-assigned this Jul 19, 2023
@ChuckHastings ChuckHastings added bug Something isn't working non-breaking Non-breaking change labels Jul 19, 2023
@ChuckHastings ChuckHastings added this to the 23.08 milestone Jul 19, 2023
@ChuckHastings ChuckHastings marked this pull request as ready for review July 19, 2023 14:19
@ChuckHastings ChuckHastings requested a review from a team as a code owner July 19, 2023 14:19
edge_partition.major_hypersparse_idx_from_major_nocheck(major);
assert(major_hypersparse_idx);
major_idx = edge_partition.major_offset_from_major_nocheck(*major_hypersparse_first) +
*major_hypersparse_idx;
Copy link
Contributor

@seunghwak seunghwak Jul 19, 2023

Choose a reason for hiding this comment

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

What's the difference between these two?

First, if ((major_hypersparse_first) && (*major_hypersparse_first < major)) should be if ((major_hypersparse_first) && (*major_hypersparse_first <= major))

And if I am not mistaken, if we're using DCSR/DCSC and major is in the hypersparse range, we are setting major_idx to major_hypersparse_idx + major_idx of the beginning of the hypersparse range. Otherwise major_idx is just major_offset.

I am not sure how these two are different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see, this isn't about major_idx, but this is about major_offset, let me take a closer look.

Copy link
Contributor

Choose a reason for hiding this comment

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

if ((major_hypersparse_first) && (*major_hypersparse_first < major))
=>
if ((major_hypersparse_first) && (major >= *major_hypersparse_first))

Otherwise, LGTM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

@jnke2016 jnke2016 left a comment

Choose a reason for hiding this comment

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

Reviewed and tested. Looks good to me.

@ChuckHastings
Copy link
Collaborator Author

/merge

@rapids-bot rapids-bot bot merged commit 803b854 into rapidsai:branch-23.08 Jul 20, 2023
52 checks passed
@ChuckHastings ChuckHastings deleted the mg_edge_bc_primitive_bug branch September 27, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants