[FEA] Add support for bitmap_view & the API of bitmap_to_csr
#2109
Conversation
- This PR is one part of the Feature of [FEA] Pre-filtered brute-force KNN rapidsai#1969 Authors: - James Rong (https://github.com/rhdong) Approvers: - Ben Frederickson (https://github.com/benfred) - Micka (https://github.com/lowener) - Corey J. Nolet (https://github.com/cjnolet)
rhdong
added
enhancement
|
Hi @lowener , thank you for your valuable review. I accepted all your review comments and modified the code. Please help review again when you feel convenient; thank you! |
|
Hi @lowener, I accepted all your review comments(3rd round) and modified the code. Please help review again when you feel convenient; thank you! |
|
Heloo @lowener, I accepted all your review comments(4th round) and modified the code. Please help review again when you feel convenient; thank you! |
| // Ensure the HBM allocated for CSR values is sufficient to handle all non-zero bitmap bits. | ||
| // An assert will trigger if the allocated HBM is insufficient when `NDEBUG` isn't defined. | ||
| // Note: Assertion is active only if `NDEBUG` is undefined. | ||
| if (lane_id == 0) { assert(nnz < indptr[num_rows]); } |
There was a problem hiding this comment.
Hi @cjnolet, as per Michael's recommendation, I'd like you to review it here. There is a trade-off for checking if the nnz of CSR is equal to the non-zero bits number of the bitmap. Option 1: checking outside the kernel by using the RAFT_EXPECTS, but we had to sync stream explicitly, which may potentially break the effort of end-users on building a pipeline to improve end-2-end performance with multi-stream in the complex industry scenarios; Option 2: use assert in the kernel and reuse the indptr[num_rows] as the non-zero bits number of bitmap, but this solution may disable by #define NDEBUG. The current implementation is Option 2. Thank you!
| template <typename bitmap_t, typename index_t, typename value_t, typename nnz_t> | ||
| void bitmap_to_csr(raft::resources const& handle, | ||
| raft::core::bitmap_view<bitmap_t, index_t> bitmap, | ||
| raft::device_csr_matrix_view<value_t, index_t, index_t, nnz_t> csr) |
There was a problem hiding this comment.
Our sparse APIs discern between strucure owning and structure preserving. The former means that the nnz is not yet known when the user creates the object. The latter means that nnz is known upon creation and any processing functions do not have the freedom to change it. One of the reasons we discern between these two is so that upon invoking a function, the user knows whether the function is supposed to compute the sparsity in addition to populating the resulting non-zero weights/elements. It doesn't look like we make that distinction here, yet we are calculating and computing the sparsity in the implementation.
There was a problem hiding this comment.
Does it mean we should calculate/get the nnz of bitmap and reallocate/allocate values by it for csr in the API?
cpp/test/sparse/convert_csr.cu
Outdated
| auto bitmap = | ||
| raft::core::bitmap_view<bitmap_t, index_t>(bitmap_d.data(), params.n_rows, params.n_cols); | ||
|
|
||
| auto csr_view = raft::make_device_compressed_structure_view<index_t, index_t, index_t>( |
There was a problem hiding this comment.
We shouldn't have to know the sparsity of the CSR when we call bitmap_to_csr. To provide a good user experienc, we should accept a structure-owning CSR so we can count the number of nonzeros in the bitmap view for the user and then initialize the csr w/ the sparsity. Please see the device_sparsity_owning_csr_matrix_view and the initialize_sparsity method.
| * @param[in] bitmap input raft::bitmap_view | ||
| * @param[out] csr output raft::device_csr_matrix_view | ||
| * @param[in] handle RAFT handle | ||
| * @param[in] bitmap input raft::bitmap_view |
There was a problem hiding this comment.
In and out are already specified on the param so we shouldn't need to duplicate those in the description. Can you please expand the descriptions, though? Instead of just "handle" and "raft::bitmap_view", it would be helpful to explain their purpose.
Also, please don't include "reference type" in the description. The caller doesn't have to care that it's a reference type. That's automatic when they pass in the argument.
There was a problem hiding this comment.
I think we should still be accepting device_structure_owning_csr_matrix here unless we intend to explicitly support both. If a user passes in a structure-preserving csr matrix, they will end up getting an error when you try to initialize the sparsity.
EDIT: Nevermind. I see where you are supporting both below.
|
Hey @cjnolet, I responded to all your comments, and all checks have been passed. Thank you! |
|
/merge |
Authors:
Approvers: