Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
tarang-jain
added
feature request
… into view-pq-quantizer
… view-pq-quantizer
tfeher
left a comment
tfeher
left a comment
There was a problem hiding this comment.
Hi Tarang, thank you for the PR! Overall looks good, but there is an issue with a copy operation that should be fixed before we can merge this.
Nitpick, but I find 'View Type PQ preprocessor' expression confusing. Could you please update the PR description to explain that this PR provides an owning and non owning variants of the vpq_dataset?
| // Copy the data from the source (data type is uint8_t, independent of MathT) | ||
| auto data_view = src.data(); | ||
| auto data = raft::make_device_matrix<uint8_t, IdxT, raft::row_major>( | ||
| res, data_view.extent(0), data_view.extent(1)); | ||
| raft::copy(data.data_handle(), | ||
| data_view.data_handle(), | ||
| data_view.size(), | ||
| raft::resource::get_cuda_stream(res)); |
There was a problem hiding this comment.
src.data() is the whole dataset. Although it is already compressed, it can still be HUGE in size. We definitely do not want to copy it. Note that the function takes src as rvalue reference, so we can just move src.data() to the new vpq_dataset and take ownership of it.
… into view-pq-quantizer
tarang-jain
added
breaking
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
Co-authored-by: Tamas Bela Feher <tfeher@nvidia.com>
|
@tfeher I have updated the PR desc |
… into view-pq-quantizer
lowener
left a comment
lowener
left a comment
There was a problem hiding this comment.
Looks very good, thanks! Just have a remark on build()
| cuvs::preprocessing::quantize::pq::quantizer<float> quantizer{ | ||
| pq_params, | ||
| cuvs::neighbors::vpq_dataset<float, int64_t>{ | ||
| std::move(vq_codebook), std::move(pq_codebook_copy), std::move(empty_data)}}; | ||
| cuvs::preprocessing::quantize::pq::vpq_codebooks<float>{ | ||
| std::make_unique<cuvs::preprocessing::quantize::pq::vpq_codebooks_owning<float>>( | ||
| std::move(vq_codebook), std::move(pq_codebook_copy))}}; |
There was a problem hiding this comment.
Use the new pq::build() with views that you're introducing in this PR. That will also get rid of the relative include of src
| auto quantizer = cuvs::preprocessing::quantize::pq::quantizer<float>( | ||
| pq_params, | ||
| cuvs::neighbors::vpq_dataset<float, int64_t>{ | ||
| raft::make_device_matrix<float, uint32_t, raft::row_major>(res, 0, 0), | ||
| std::move(pq_codebook), | ||
| raft::make_device_matrix<uint8_t, int64_t, raft::row_major>(res, 0, 0)}); | ||
| cuvs::preprocessing::quantize::pq::vpq_codebooks<float>{ | ||
| std::make_unique<cuvs::preprocessing::quantize::pq::vpq_codebooks_owning<float>>( | ||
| raft::make_device_matrix<float, uint32_t, raft::row_major>(res, 0, 0), | ||
| std::move(pq_codebook))}); |
There was a problem hiding this comment.
Use the new pq::build() with views
| vpq_codebooks_owning(raft::device_matrix<math_type, uint32_t, raft::row_major>&& vq_code_book, | ||
| raft::device_matrix<math_type, uint32_t, raft::row_major>&& pq_code_book) |
There was a problem hiding this comment.
Nitpick: Would it make sense to have the vq as optional in both struct? It would follow the other functions parameter order too if it's by default nullopt.
4 participants
Separate VPQ codebooks from encoded data by introducing
vpq_codebooks<MathT>(PIMPL with owning/view variants).vpq_datasetis now a simple{codebooks, data}struct.quantizer<T>holds onlyvpq_codebookssince it doesn't need encoded data. Adds a view-typepq::build()overload for pre-computed external codebooks.