Define k-core API and tests

[ChuckHastings]

Define k-core C++ API and tests.

Closes #2631
Closes #2632
Closes #2633
Closes #2635

[codecov-commenter]

Codecov Report

Base: 60.04% // Head: 60.04% // No change to project coverage 👍

Coverage data is based on head (59bce1d) compared to base (3eb2b40).
Patch has no changes to coverable lines.

❗ Current head 59bce1d differs from pull request most recent head 3f7db87. Consider uploading reports for the commit 3f7db87 to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##           branch-22.10    #2712   +/-   ##
=============================================
  Coverage         60.04%   60.04%           
=============================================
  Files               111      111           
  Lines              6184     6184           
=============================================
  Hits               3713     3713           
  Misses             2471     2471           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[naimnv]

Would it be a better idea to pass bool transpose=false as template parameter and then use transpose as a named parameter to the function instead of implicit false?

[ChuckHastings]

We try to avoid excess compilation of different templated functions, as this increased compile time.

Most of our functions are implemented based on an assumption about whether the graph is stored transposed or not. For example, pagerank assumes that store_transposed=true. Implementing pagerank if store_transposed=false would result in a great deal more synchronization (and communication in a multi-gpu environment). There are a few examples that will work on a graph in either orientation.

Supporting this would require providing an implementation that would work with either orientation of the graph. It seems like as long as this matches the requirement for core_number (which has to be called first) then just one orientation is sufficient.

[naimnv]

Same here as mentioned in my previous comment.

[ChuckHastings]

See above reaction.

[seunghwak]

I assume this comment is outdated.

[ChuckHastings]

Fixed

[seunghwak]

Something to think about...

So, we're passing core numbers here. So in this case, should we really have both this function and induced subgraph?

seeing core numbers, we can easily extract a vertex list in a single thrust call (e.g. thrust::copy_if). Then, we can pass a vertex list to induced subgraph.

Not sure this function adds enough convenience to justify increase in compile time/binary size.

[seunghwak]

Maybe OK, if this calls explicitly instantiated induced_subgraph, increases in compile time/binary size might be minimal...

Still debating between whether this function should take core_numbers or just call core_numbers internally.... (to maximize user convenience, if not, a user can just separately call core_numbers, something like thurst::copy_if, and induced_subgraph).

[ChuckHastings]

I had that debate internally. Ultimately I concluded that if we're going to expose a complete k-core implementation at the python level we should probably expose it at the lower levels as well. It seems like something that would be useful.

I do wonder if we should make passing in core_numbers optional, and if they are not passed then also call core_numbers. This would allow a simple caller that just wants to extract a 3-core from the graph to be able to call the function directly and get the complete answer, while a more sophisticated case might call core_number once and reuse the result to extract the different subgraphs as required.

[seunghwak]

Yeah... +1 for taking core_numbers as std::optional.

Still a bit not-sure about "Ultimately I concluded that if we're going to expose a complete k-core implementation at the python level we should probably expose it at the lower levels as well."

At the python layer, user convenience might be more important than anything else. And providing an additional python function that internally calls core_number, find vertex list, and induced subgraph has no toll on compile time/binary size as python is an interpreted language.

Not sure we should provide the same level of convenience for C++ users, as we assume C++ users are more advanced and they might be OK about finding k-core by composing core_numbers and induced subgraph.

But this is a much bigger topic, and we may have this discussion again in the future.

[seunghwak]

And another thing to consider is that this may incur some sort of dependency within a same algorithm level in our C++ software hierarchy. Most algorithms are composed of primitives and thrust calls. But now algorithms are composed of primitives, thrust calls, and other algorithms. I am not sure what kind of complications this can incur in the future.

Composing C++ algorithms in the C layer might be OK, but I am not sure whether we should support use cases that can be supported simply by composing few existing algorithms in the C++ level.

[ChuckHastings]

Fair point.

I will change core_numbers to be optional.

We can explore dropping the k_core from the C++ layer if having the algorithms depend on other algorithms becomes an issue. I see the point that adding it at the C layer may be sufficient.

[jnke2016]

I don't see where these k-core functions are implemented: cugraph_core_result_get_src_vertices, cugraph_core_result_get_dst_vertices, cugraph_core_result_get_weights. Please can you point me to those? I am getting the error below in pylibcugraph

cugraph_core_result_get_src_vertices' is not a constant, variable or function identifier

[ChuckHastings]

Good catch, Joseph. Adding the missing functions.

[ChuckHastings]

I see another issue, need to fix something else also.

[jnke2016]

Ok thanks. It looks like cugraph_k_core too is missing

[ChuckHastings]

rerun tests

[ChuckHastings]

@gpucibot merge