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.

Sign up for GitHub

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

Jump to bottom

Define C API for eigenvector centrality #2180

Merged
merged 6 commits into from
Apr 6, 2022
Merged

Define C API for eigenvector centrality #2180

merged 6 commits into from
Apr 6, 2022

Conversation

ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Mar 30, 2022 •
edited
Loading

This PR defines the C API for eigenvector centrality. It is the first of a series of PRs to address #2146

It also does the following:

  • Renames cugraph_pagerank_result_t to cugraph_centrality_result_t to allow it to be reused for all centrality algorithms
  • Reorganizes the code for the results into a separate file
  • Provides an initial implementation of eigenvector centrality that can be used for testing the API. NOTE: It literally calls pagerank with some hardcoded parameters. The return formats for the algorithms are the same, so this will mimic the API behavior (although the scores will be different once eigenvector centrality is implemented
  • Provided a C unit test that validates the results (will need to be updated to validate eigenvector centrality results once the real algorithm is in place)

Note that this is a breaking change since it modifies the type of the pagerank result.

@ChuckHastings ChuckHastings requested review from a team as code owners March 30, 2022 22:22
@ChuckHastings ChuckHastings self-assigned this Mar 30, 2022
@ChuckHastings ChuckHastings added 3 - Ready for Review feature request New feature or request non-breaking Non-breaking change labels Mar 30, 2022
@ChuckHastings ChuckHastings added this to the 22.06 milestone Mar 30, 2022
@ChuckHastings
Copy link
Collaborator Author

rerun tests

seunghwak
seunghwak reviewed Apr 4, 2022
View reviewed changes
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.

Looks good to me except for few nitpicking comments.

cpp/include/cugraph_c/algorithms.h Outdated Show resolved Hide resolved
cpp/src/c_api/centrality_result.hpp Outdated Show resolved Hide resolved
cpp/src/c_api/eigenvector_centrality.cpp Outdated Show resolved Hide resolved
cpp/src/c_api/eigenvector_centrality.cpp Show resolved Hide resolved
cpp/src/c_api/pagerank.cpp Outdated Show resolved Hide resolved
@ChuckHastings ChuckHastings requested a review from rlratzel April 5, 2022 15:36
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2022 •
edited
Loading

Codecov Report

Merging #2180 (8a0890b) into branch-22.06 (017baab) will decrease coverage by 1.54%.
The diff coverage is 61.53%.

❗ Current head 8a0890b differs from pull request most recent head 6ac25df. Consider uploading reports for the commit 6ac25df to get more accurate results

@@               Coverage Diff                @@
##           branch-22.06    #2180      +/-   ##
================================================
- Coverage         72.41%   70.86%   -1.55%     
================================================
  Files               157      170      +13     
  Lines             10496    11036     +540     
================================================
+ Hits               7601     7821     +220     
- Misses             2895     3215     +320
Impacted Files Coverage Δ
...aph/cugraph/dask/sampling/neighborhood_sampling.py 0.00% <0.00%> (ø)
.../cugraph/cugraph/experimental/compat/nx/DiGraph.py 0.00% <0.00%> (ø)
...on/cugraph/cugraph/experimental/compat/nx/Graph.py 0.00% <0.00%> (ø)
...thon/cugraph/cugraph/experimental/dask/__init__.py 0.00% <0.00%> (ø)
python/cugraph/cugraph/tests/dask/test_mg_hits.py 0.00% <0.00%> (ø)
...ugraph/tests/dask/test_mg_neighborhood_sampling.py 0.00% <0.00%> (ø)
.../cugraph/cugraph/tests/dask/test_mg_replication.py 0.00% <0.00%> (-33.34%) ⬇️
python/cugraph/cugraph/dask/link_analysis/hits.py 18.36% <18.36%> (ø)
...h/pylibcugraph/tests/test_neighborhood_sampling.py 20.83% <20.83%> (ø)
python/cugraph/cugraph/sampling/node2vec.py 81.81% <69.23%> (-9.30%) ⬇️
... and 32 more

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 017baab...6ac25df. Read the comment docs.

@ChuckHastings ChuckHastings added breaking Breaking change and removed non-breaking Non-breaking change labels Apr 5, 2022
rlratzel
rlratzel approved these changes Apr 5, 2022
View reviewed changes
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.

Just some minor observations which need not hold up approval.

cpp/src/c_api/centrality_result.cpp Outdated Show resolved Hide resolved
cpp/src/c_api/eigenvector_centrality.cpp Outdated Show resolved Hide resolved
@ChuckHastings ChuckHastings mentioned this pull request Apr 5, 2022
Merged
@ChuckHastings ChuckHastings requested a review from a team as a code owner April 5, 2022 20:46
@ChuckHastings
Copy link
Collaborator Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 9956084 into rapidsai:branch-22.06 Apr 6, 2022
@betochimas betochimas mentioned this pull request Apr 6, 2022
Merged
betochimas added a commit to betochimas/cugraph that referenced this pull request Apr 6, 2022
@betochimas
Merge pull request #39 from betochimas/branch-22.06
66ba355 
Define C API for eigenvector centrality (rapidsai#2180)
This was referenced Apr 24, 2022
Closed
Merged
rapids-bot bot pushed a commit that referenced this pull request May 27, 2022
@betochimas
Add Eigenvector Centrality to pylibcugraph, cugraph APIs (#2255)
6d0239d 
This PR:

1.  Adds Eigenvector Centrality to the pylibcugraph and cugraph software stacks, which started from #2180 and is followed up by future PRs in order to close #2146  
2. Minor improvements to pylibcugraph Katz Centrality
3. Added functionality to `test_doctests.py` so that certain docstrings can be skipped on different architecture configs (such as ktruss in CUDA 11.4) 
4. Added undirected/directed versions of graph example used in C tests in `datasets`
5. Removed cugraph copy of warning wrapper from pylibcugraph and have it call the pylibcugraph version
6. Testing for both Python eigenvector centrality wrappers

This PR is identical to #2243, just that the name of the branch is different

Authors:
  - https://github.com/betochimas
  - Chuck Hastings (https://github.com/ChuckHastings)

Approvers:
  - Rick Ratzel (https://github.com/rlratzel)

URL: #2255
@ChuckHastings ChuckHastings deleted the fea_eigenvector_centrality_definition branch August 4, 2022 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seunghwak seunghwak seunghwak approved these changes

@rlratzel rlratzel rlratzel approved these changes

Assignees

@ChuckHastings ChuckHastings

Labels
breaking Breaking change feature request New feature or request
Projects
None yet
Milestone
22.06
Development

Successfully merging this pull request may close these issues.
4 participants
@ChuckHastings @codecov-commenter @rlratzel @seunghwak