Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
matrix::select_k: move selection and warp-sort primitives #1085
matrix::select_k: move selection and warp-sort primitives #1085
Changes from 1 commit
39c10a9
6cda736
c5631bf
fb88433
f64325b
20d01d7
4813bae
6cdb79a
870fc86
2af45bf
5b336ee
b3e5d9c
164157b
69c81dd
d64b12b
9d4476a
3e40435
e20578e
b2c79f5
98e2c2a
af4c146
471828e
f6ff223
066208d
aeaa1ef
685b6bf
5c42209
2cea50d
a31e61e
a8c5a70
8a5978b
8e58cab
a01a75f
c6256b7
c25e859
6e56106
c78d9b0
a55a6cb
307b113
c0ce160
e2cc7ad
dc3043c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer not to mix/include test files in benchmark code (and vice versa)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a great deal of shared code in this file! Would you consider adding some shared test+bench header-only component? As far as I know, at the moment we have only one more place where we explicitly refer to a test header from the benchmarks (bench/neighbors/refine.cu#L39), but there could definitely be more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If these are needed to be tested individually, I think we should expose public APIs for them. in general, we should only be benchmarking/testing public APIs.
(I know we are benchmarking the balanced kmeans which is currently in detail, but we're working to move this out into the public).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation variants in radix- and warpsort- files weren't and were never meant to be public. We've been automatically selecting the implementation in the public wrappers based on the runtime arguments. I see a few issues in making all of them public:
At the same time, we cannot remove the tests for individual variants, because the selection logic of the public wrapper makes it impossible to test full input ranges for every variant.
And we really need the individual benchmarks, so that we can compare the implementations and then adjust the selection logic (and repeat this as new implementations and optimizations arrive).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this begs the questions as to how much we should expose and how advanced we should consider our users to be. I'm not opposed to keeping these internal if we have a good well-defined (and well-documented) set of rules guiding the choice, resource, and runtime trade-offs of each.
I'm not saying we should remove the tests for individual variants but testing internals does not replace the need to test the public APIs comprehensively and exhaustively with all available options. After all, the users interact with the public APIs and the quality of their user experience should be better guaranteed by successfully passing tests.
The problem that arises from testing only internals and not also exhaustively testing the public APIs is that we gain a false sense of correctness when a developer makes changes to the public APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the moment, we have 8 implementations/combinations. At least one more (new advances in radix-based select) in the works, and some of the current ones in
warp_sort
may be strictly worse than others. I hope we can keep all of them in detail, so that we can improve the implementation selection logic and remove some of the implementations in future.Perhaps, it's not clear behind the templates, but the public API interface is the most tested in
test/matrix/select_k.cu
. Besides the simple tests against static in-out values, we compare the outputs of the various implementations against the public one; this should ensure that the public-selected-implementation is compared against other non-selected implementations for various parameter ranges.I think, this should cover the public interface well, but I'd be happy to add more tests if you have any further suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get this out of
raft::sparse
at some point. Nothin to do in this PR but just making a note.