-
Notifications
You must be signed in to change notification settings - Fork 300
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 new primitives: compute_in|out_degrees, compute_in|out_weight_sums to graph_view_t #1394
Merged
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c7c5d38
rename compute_major_degree to compute_major_degrees
seunghwak 5919ddc
add SG version of compute_major_degrees
seunghwak 8e6e6b3
add compute_in|out_degrees, compute_in|out_weight_sums
seunghwak 6644f6b
replace custom code to compute out weight sums with graph_view_t's co…
seunghwak e6f50a5
Merge branch 'branch-0.19' of github.com:rapidsai/cugraph into fea_de…
seunghwak be518b0
Merge branch 'branch-0.19' of github.com:rapidsai/cugraph into fea_de…
seunghwak dc00022
bug fix
seunghwak 9694134
add degree & weight-sum tests
seunghwak 04ab2ca
renmove std::move that prevents NRVO in return statements
seunghwak 64b0f8c
clang-format
seunghwak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
add compute_in|out_degrees, compute_in|out_weight_sums
- Loading branch information
commit 8e6e6b334c935dbbc6ddf886b5d1ed8162a267af
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we have started to use minor/major terminology but seeing this here used concurrently with src/dst and in/out terminology makes me wonder if that's really helping. We should try to stick to the smallest possible set of names that refer to the same thing.
Reconciling this is beyond this PR, we should have a separate issue/PR for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the origin of minor/major might be from https://github.com/rapidsai/cugraph/blob/branch-0.19/cpp/include/patterns/copy_v_transform_reduce_in_out_nbr.cuh and this terminology has been used since the start of graph primitives.
This was introduced mainly for source code reuse, noting that iterating over incoming edges when the graph adjacency matrix is stored as is (CSR) and iterating over outgoing edges when the graph adjacency matrix stored as transposed (CSC) can be implemented using the same code.
Yeah... we may open a several issue and we may need to go through more intense discussion on terminologies as these are now used all over the graph primitives. If there is a clear benefit to make a switch, yes we should make a switch, but I assume there should be a very strong justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this might be a much bigger discussion, but we may later more intensively investigate the performance benefit of storing graph adjacency matrix as transposed.
This was mainly introduced to avoid atomics, but AFAIK, GPU's performance on atomics has improved drastically in the past some years and if the performance gain is not large enough, we may reconsider this issue. If the performance gain is marginal, dropping this will significantly simplify our codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this terminology issue will become irrelevant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me. Thanks for elaborating. Let's open an issue to keep track of this.