[Opt] Expose the detail::popc as public API#2346
[Opt] Expose the detail::popc as public API#2346rapids-bot[bot] merged 4 commits intorapidsai:branch-24.08from
detail::popc as public API#2346Conversation
| * @param[out] counter Device scalar view to store the number of bits that are set to 1. | ||
| */ | ||
| template <typename value_t, typename index_t> | ||
| void popc(const raft::resources& res, |
There was a problem hiding this comment.
To be quite honest, I'm not sure this is something that belongs in raft::core.
Also, I'm a little confused by the max_len- can you explain why this additional argument is needed? This API seems a little implementation-specific, which makes me wonder if it should be more strongly coupled to the bitset/bitmap.
There was a problem hiding this comment.
The original wrap of popc is here: https://github.com/rapidsai/raft/blob/branch-24.08/cpp/include/raft/core/detail/mdspan_util.cuh#L48. But it can only calc nonzero bits in one int32/int64, while this one can calculate on an integer array. Would you have some recommendations on the namespace? Maybe the raft::linalg is another choice.
The max_len should be required to indicate the max bits to be processed. For example, we have 100 bits needed to be calculated and at least 4 uint32_ts to present them(get 128 bits physically). In this situation, a max_len or similar parameter is needed to tell the popc to stop on the right bits.
There was a problem hiding this comment.
This API is mainly for clean code. The detail::popc includes at least 10 lines of code because it has to process the corner cases. If we were using the original code, we would have to copy that block of code anywhere, that is ugly.
There was a problem hiding this comment.
Since this is a computational function, I don't think it belongs in raft/core. I question whether or not it actually belongs in raft/matrix or in raft/utils, though. Hmm.
There was a problem hiding this comment.
I prefer raft/utils as it seems more reasonable. If you agree, I will move it there. @cjnolet
rhdong
added
enhancement
|
/merge |
divyegala
added
breaking
rapidsai/raft#2346 introduced a breaking change in the API. This PR fixes the API usage. Authors: - Divye Gala (https://github.com/divyegala) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: #249
rapidsai/raft#2346 introduced a breaking change in the API. This PR fixes the API usage. Authors: - Divye Gala (https://github.com/divyegala) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#249
- For resolving the issue of cuVS: rapidsai/cuvs#158 Authors: - rhdong (https://github.com/rhdong) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: rapidsai#2346
3 participants
raft::xx::detail:calling for prefiltered brute-force toraft::xxnamespace cuvs#158