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

CAGRA: Separate graph index sorting functionality from prune function #1471

Merged
merged 16 commits into from
May 10, 2023

Conversation

enp1s0
Copy link
Member

@enp1s0 enp1s0 commented Apr 28, 2023

Changes

This PR separates the graph index sorting functionality from the CAGRA pruning function and creates a new function. (Related issue: #1446)

Unit test

I have included a new unit test for the sorting function. The test utilizes a separate dataset from the one used in the CAGRA main test to avoid the effect of rounding errors during norm computation between two vectors in the dataset. More details are in the source code.
https://github.com/enp1s0/raft/blob/ea6c449c260895e9125a591a4848eed06f5b72c4/cpp/test/neighbors/ann_cagra.cuh#L93-L96

Issue

Close #1446

@enp1s0 enp1s0 requested a review from a team as a code owner April 28, 2023 03:11
@github-actions github-actions bot added the cpp label Apr 28, 2023
@enp1s0 enp1s0 self-assigned this Apr 28, 2023
@enp1s0 enp1s0 added enhancement New feature or request non-breaking Non-breaking change improvement Improvement / enhancement to an existing function and removed enhancement New feature or request labels Apr 28, 2023
@enp1s0
Copy link
Member Author

enp1s0 commented Apr 28, 2023

@tfeher Can you check the PR and let me know if there are any problems?

Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @enp1s0 for the PR! It looks good overall, below you will find a few small comments (and just as a reference, I have also linked two existing issues).

cpp/include/raft/neighbors/cagra.cuh Show resolved Hide resolved
cpp/test/neighbors/ann_cagra.cuh Outdated Show resolved Hide resolved
cpp/test/neighbors/ann_cagra.cuh Outdated Show resolved Hide resolved
@enp1s0
Copy link
Member Author

enp1s0 commented May 8, 2023

@tfeher Thank you for your comments.
I have added a code example of sort_knn_graph and fixed the stream synchronization in the test code. I'll fix the memory allocation and multi-GPU support in another PR because there will be a lot of code changes.

Copy link
Contributor

@tfeher tfeher left a comment

Choose a reason for hiding this comment

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

Thanks @enp1s0 for addressing the issues. The PR looks good to me.

cpp/test/neighbors/ann_cagra.cuh Show resolved Hide resolved
@enp1s0 enp1s0 force-pushed the fea-cagra-sort-knn-graph branch from 068d832 to b447bdd Compare May 9, 2023 08:55
@enp1s0
Copy link
Member Author

enp1s0 commented May 10, 2023

I have changed the test case parameters to satisfy the following requirements in int8 and uint8 tests.

https://github.com/rapidsai/raft/blob/20141120f6ae2fd9ea1ea0648507af04594921ec/cpp/include/raft/neighbors/detail/cagra/cagra_build.cuh#LL52C45-L52C45

RAFT_EXPECTS(
    dataset.extent(1) * sizeof(DataT) % 8 == 0,
    "Dataset rows are expected to have at least 8 bytes alignment. Try padding feature dims.");

@cjnolet
Copy link
Member

cjnolet commented May 10, 2023

/merge

@rapids-bot rapids-bot bot merged commit cc4a76b into rapidsai:branch-23.06 May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp 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.

[FEA] Separate index sorting from CAGRA graph pruning
3 participants