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

Define triangle_count C API #2271

Merged

Conversation

ChuckHastings
Copy link
Collaborator

This defines the C API for triangle counting.

This PR is independent of #2253 and can be merged independently. The change here defines the C API and returns the CUGRAPH_NOT_IMPLEMENTED error when called. Once #2253 is completed and merged, a follow-up PR will fill in the C API implementation for triangle counting (although the code is written and untested in this PR).

@ChuckHastings ChuckHastings requested review from a team as code owners May 13, 2022 17:46
@ChuckHastings ChuckHastings self-assigned this May 13, 2022
@ChuckHastings ChuckHastings added 3 - Ready for Review improvement Improvement / enhancement to an existing function non-breaking Non-breaking change feature request New feature or request and removed improvement Improvement / enhancement to an existing function labels May 13, 2022
@ChuckHastings ChuckHastings added this to the 22.06 milestone May 13, 2022
@ChuckHastings ChuckHastings added improvement Improvement / enhancement to an existing function and removed feature request New feature or request labels May 13, 2022
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

LGTM (except for one comment about documentation).

* @param [in] handle Handle for accessing resources
* @param [in] graph Pointer to graph. NOTE: Graph might be modified if the storage
* needs to be transposed
* @param [in] start Device array of vertices we want to count triangles for
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment saying "if this is NULL, the entire set of vertices is assumed"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will fix in next push.

@codecov-commenter
Copy link

Codecov Report

Merging #2271 (faa7196) into branch-22.06 (916dc5c) will not change coverage.
The diff coverage is n/a.

❗ Current head faa7196 differs from pull request most recent head 9381e36. Consider uploading reports for the commit 9381e36 to get more accurate results

@@              Coverage Diff              @@
##           branch-22.06    #2271   +/-   ##
=============================================
  Coverage         63.97%   63.97%           
=============================================
  Files               100      100           
  Lines              4436     4436           
=============================================
  Hits               2838     2838           
  Misses             1598     1598           

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 916dc5c...9381e36. Read the comment docs.

Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

LGTM. I focused on the public API part and less on the (commented out) implementation. I had one questions which need not hold up approval.

* the entire set of vertices in the graph is processed
* @param [in] do_expensive_check
* A flag to run expensive checks for input arguments (if set to true)
* @param [in] result Output from the triangle_count call
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is result labelled in but error is an out? I figured they'd both be out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Should both be out. I will correct in a follow-up PR.

@ChuckHastings
Copy link
Collaborator Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 046cbd2 into rapidsai:branch-22.06 May 16, 2022
betochimas added a commit to betochimas/cugraph that referenced this pull request May 16, 2022
@ChuckHastings ChuckHastings deleted the fea_add_triangle_counting_c_api branch August 4, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants