Conversation
aamijar
added
non-breaking
c/include/cuvs/preprocessing/pca.h
Outdated
| */ | ||
| enum cuvsPcaSolver { | ||
| /** Covariance + divide-and-conquer eigen decomposition */ | ||
| PCA_COV_EIG_DQ = 0, |
There was a problem hiding this comment.
enums should be CUVS prefixed
| #endif | ||
|
|
||
| /** | ||
| * @defgroup preprocessing_c_pca C API for PCA (Principal Component Analysis) |
c/tests/preprocessing/run_pca_c.c
Outdated
| output_tensor.manager_ctx = NULL; | ||
| output_tensor.deleter = NULL; | ||
|
|
||
| cuvsPcaFitTransform(res, |
There was a problem hiding this comment.
Can you add an option to test fit() and predict() independantly too?
c/tests/preprocessing/pca_c.cu
Outdated
| cudaMemcpyDeviceToDevice, | ||
| stream)); | ||
|
|
||
| run_pca(n_rows, |
There was a problem hiding this comment.
Sync stream before calling this function as there are no guarantee that the inputs are populated before pca::fit() is called.
Or you can create the C resource in this function and pass it to run_pca
c/tests/preprocessing/pca_c.cu
Outdated
| raft::random::RngState rng(1234ULL); | ||
| raft::random::uniform(handle, rng, input.data(), n_rows * n_cols, -1.0f, 1.0f); | ||
|
|
||
| RAFT_CUDA_TRY(cudaMemcpyAsync(input_copy.data(), |
There was a problem hiding this comment.
Use C++ wrapper for memcpy and streamsync
c/tests/preprocessing/run_pca_c.c
Outdated
| DLManagedTensor input_tensor; | ||
| input_tensor.dl_tensor.data = input_data; | ||
| input_tensor.dl_tensor.device.device_type = kDLCUDA; | ||
| input_tensor.dl_tensor.device.device_id = 0; | ||
| input_tensor.dl_tensor.ndim = 2; | ||
| input_tensor.dl_tensor.dtype.code = kDLFloat; | ||
| input_tensor.dl_tensor.dtype.bits = 32; | ||
| input_tensor.dl_tensor.dtype.lanes = 1; | ||
| int64_t input_shape[2] = {n_rows, n_cols}; | ||
| int64_t input_strides[2] = {1, n_rows}; | ||
| input_tensor.dl_tensor.shape = input_shape; | ||
| input_tensor.dl_tensor.strides = input_strides; | ||
| input_tensor.dl_tensor.byte_offset = 0; | ||
| input_tensor.manager_ctx = NULL; | ||
| input_tensor.deleter = NULL; |
There was a problem hiding this comment.
Can you use cuvs::core::detail::to_dlpack() for those? That should reduce a lot the number of lines so this function could fit in the same test file
c/tests/preprocessing/run_pca_c.c
Outdated
| res, params, &trans_tensor, &comp_tensor, &sv_tensor, &mu_tensor, &output_tensor); | ||
|
|
||
| cuvsPcaParamsDestroy(params); | ||
| cuvsResourcesDestroy(res); |
There was a problem hiding this comment.
Sync stream before destroying the resource, there is no guarantee that the work is complete
robertmaynard
left a comment
robertmaynard
left a comment
There was a problem hiding this comment.
Approving CMake changes
|
Hi @lowener, I think all your comments have been addressed. |
3 participants
Resolves #1977 and Resolves #1994