Add Katz Centrality to the C API #2192
Conversation
ChuckHastings
added
2 - In Progress
improvement
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #2192 +/- ##
================================================
- Coverage 72.41% 69.40% -3.01%
================================================
Files 157 170 +13
Lines 10496 11036 +540
================================================
+ Hits 7601 7660 +59
- Misses 2895 3376 +481
Continue to review full report at Codecov.
|
| * @return type erased array view of centrality values | ||
| */ | ||
| cugraph_type_erased_device_array_view_t* cugraph_centrality_result_get_values( | ||
| cugraph_centrality_result_t* result); |
There was a problem hiding this comment.
Isn't this an update happened in one of the previous PRs? It seems like this PR includes updates for PageRank, HITS, and so on in addition to the updates for Katz. Is this because this PR is showing updates from other PRs or these are indeed new updates in this PR?
There was a problem hiding this comment.
I felt like the algorithms.h file was getting too large to keep track of. This PR (as well as #2185) starts moving like algorithms into a separate file. In this case I'm moving all of the centrality based algorithms into centrality_algorithms.h.
So the definition of this function was in an earlier PR, this is just moving it to a new file.
There was a problem hiding this comment.
Gotcha, makes sense. Not for this PR, but to be consistent, PageRank is currently under link_analysis. Maybe in a separate PR, shouldn't we better move PageRank to the centrality directory? (in C/C++/Python).
There was a problem hiding this comment.
There was a problem hiding this comment.
Yeah...and we also should start thinking about splitting algorithms.hpp to put the C & C++ parts in sync.
There was a problem hiding this comment.
Unlike the previous PR where you defined the centrality result types in both C and pylib, here you're splitting up algorithms.h into itself and centrality_algorithms.h. I think it would be better and easier to follow along if I do the same in my pylibcugraph wrapper PR for algorithms.pxd and centrality_algorithms.pxd. It would likely prevent a merge conflict and I can keep pulling from your branch whenever changes are being made
There was a problem hiding this comment.
@betochimas - that makes sense. Did you want me to split that in this PR, or are you going to handle it in #2201?
There was a problem hiding this comment.
@ChuckHastings I can handle it in #2201
There was a problem hiding this comment.
yep, I think @betochimas is saying this too: we've been keeping the .pxd files in pylibcugraph/_cugraph_c in sync with the C API headers to make it easy to follow, so we'd want the same reorg in pylibcugraph. I think it can be done in #2201.
| * @return type erased array view of centrality values | ||
| */ | ||
| cugraph_type_erased_device_array_view_t* cugraph_centrality_result_get_values( | ||
| cugraph_centrality_result_t* result); |
There was a problem hiding this comment.
yep, I think @betochimas is saying this too: we've been keeping the .pxd files in pylibcugraph/_cugraph_c in sync with the C API headers to make it easy to follow, so we'd want the same reorg in pylibcugraph. I think it can be done in #2201.
|
@gpucibot merge |
…raph (#2201) This PR adds Single-GPU (SG) Katz Centrality to the pylibcugraph software stack, as well as refactoring SG Katz Centrality for the cugraph software stack. Both Katz Centrality algorithms are wrappers calling a lower level version of the algorithm, originally based on the C API. This PR directly depends on #2192, and combined with it, should close #1759. Authors: - https://github.com/betochimas - Chuck Hastings (https://github.com/ChuckHastings) Approvers: - Rick Ratzel (https://github.com/rlratzel) URL: #2201
Add Katz Centrality to the C API. This will allow the work for #1759 to be done using the C API which should make things simpler and avoid doing the work twice.
This PR both defines and implements the Katz Centrality algorithm through the C API.