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

Trie #350

Draft
wants to merge 41 commits into
base: dev
Choose a base branch
from
Draft

Trie #350

wants to merge 41 commits into from

Conversation

amukkara
Copy link
Contributor

@amukkara amukkara commented Aug 11, 2023

Trie and associated bit-vector code with tests

@GPUtester
Copy link

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@rapids-bot
Copy link

rapids-bot bot commented Aug 11, 2023

Pull requests from external contributors require approval from a NVIDIA organization member with write permissions or greater before CI can begin.

@amukkara amukkara marked this pull request as ready for review August 11, 2023 17:50
@PointKernel
Copy link
Member

ok to test

@amukkara amukkara mentioned this pull request Aug 17, 2023
@amukkara amukkara marked this pull request as draft August 18, 2023 05:21
@sleeepyjack
Copy link
Collaborator

/ok to test

@copy-pr-bot
Copy link

copy-pr-bot bot commented Aug 30, 2023

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

PointKernel added a commit that referenced this pull request Sep 8, 2023
This PR adds `dynamic_bitset` code that will be used in Trie data
structure. Trie will be integrated in a separate PR #350.

Since `dynamic_bitset` is not intended to be part of public-facing API,
all files (.cuh and .inl) are located in
include/cuco/detail/trie/dynamic_bitset

Tests are added in tests/dynamic_bitset

---------

Co-authored-by: Yunsong Wang <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@sleeepyjack
Copy link
Collaborator

/ok to test

@sleeepyjack
Copy link
Collaborator

sleeepyjack commented Sep 12, 2023

@amukkara can you sign all of your commits in this PR (starting at c1b448e)? Easiest way to do this is do an interactive rebase, pick->edit every commit that is not signed, and then do a git commit --amend --no-edit -S && git rebase --continue until you land at HEAD.

@amukkara
Copy link
Contributor Author

@amukkara can you sign all of your commits in this PR (starting at c1b448e)? Easiest way to do this is do an interactive rebase, pick->edit every commit that is not signed, and then do a git commit --amend --no-edit -S && git rebase --continue until you land at HEAD.

Done

Copy link
Collaborator

@sleeepyjack sleeepyjack left a comment

Choose a reason for hiding this comment

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

Reviewed just the benchmarks


distribution::unique lengths_dist;
distribution::gaussian labels_dist{0.5};
generate_labels(labels, offsets, num_keys, max_key_length, lengths_dist, labels_dist);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be placed into some namespace, e.g., ´cuco::test::`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added namespace coco::test::trie 025c59a

#include <defaults.hpp>
#include <utils.hpp>

#include "../../tests/trie/trie_utils.hpp"
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use the system path here, i.e., <cuco/tests/…>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<cuco/tests/..> does not compile. Using <../tests/> in recent commit.

Should we change top-level CMakeLists or benchmarks/CMakeLists to access <cuco/tests/..> from benchmarks?

benchmarks/trie/dynamic_bitset/find_next_bench.cu Outdated Show resolved Hide resolved
benchmarks/trie/insert_bench.cu Outdated Show resolved Hide resolved
auto const num_keys = state.get_int64_or_default("NumKeys", 100 * 1000);
auto const max_key_length = state.get_int64_or_default("MaxKeyLength", 10);

using LabelType = int;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to make this a configurable parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

7f1d083 makes LabelType configurable. Testing char, int for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants