Skip to content
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

Add pairwise_distance api's for C, Python and Rust #142

Merged
merged 6 commits into from
May 23, 2024

Conversation

benfred
Copy link
Member

@benfred benfred commented May 21, 2024

No description provided.

@benfred benfred added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 21, 2024
@benfred benfred requested review from a team as code owners May 21, 2024 23:48
@benfred benfred marked this pull request as draft May 22, 2024 00:05
The C++ api includes the distance.h, and having a dlpack.h include breaks the build
since we only add for C code
@benfred benfred marked this pull request as ready for review May 22, 2024 04:28
Copy link
Member

@cjnolet cjnolet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think overall this PR looks great. I'm pre-approving, but do want to make sure we verify the behavior of the C api for unsupported type combinations. Maybe you've already done this, but I just want to avoid any unintended behaviors.

* @param[in] metric distance to evaluate
* @param[in] metric_arg metric argument (used for Minkowski distance)
*/
cuvsError_t cuvsPairwiseDistance(cuvsResources_t res,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks so simple and elegant to use!

cpp/src/distance/pairwise_distance_c.cpp Show resolved Hide resolved
auto y_mds = cuvs::core::from_dlpack<mdspan_type>(y_tensor);
auto distances_mds = cuvs::core::from_dlpack<distances_mdspan_type>(distances_tensor);

cuvs::distance::pairwise_distance(*res_ptr, x_mds, y_mds, distances_mds, metric, metric_arg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know that is always going to use the float (instantiated) version?

cpp/test/distance/pairwise_distance_c.cu Show resolved Hide resolved
distances_tensor.dl_tensor.strides = NULL;

// run pairwise distances
cuvsPairwiseDistance(res, &dataset_tensor, &queries_tensor, &distances_tensor, metric, 2.0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you haven't done it before, can you try this w/ double? (Just making sure we don't get any unnexpected surprises from the UX or the compile times / binary size).

@@ -30,9 +30,10 @@ from cuvs.distance_type cimport cuvsDistanceType
from pylibraft.common import auto_convert_output, cai_wrapper, device_ndarray
from pylibraft.common.cai_wrapper import wrap_array
from pylibraft.common.interruptible import cuda_interruptible
from pylibraft.distance.pairwise_distance import DISTANCE_TYPES
from pylibraft.neighbors.common import _check_input_array
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably need to bring this over at some point too (no rush for 24.06 since we already depend on pylibraft.

@cjnolet
Copy link
Member

cjnolet commented May 23, 2024

/merge

@rapids-bot rapids-bot bot merged commit 4b260aa into rapidsai:branch-24.06 May 23, 2024
54 checks passed
difyrrwrzd added a commit to difyrrwrzd/cuvs that referenced this pull request Aug 10, 2024
@benfred benfred deleted the pairwise_distances_api branch September 5, 2024 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp improvement Improves an existing functionality non-breaking Introduces a non-breaking change Python
Projects
Development

Successfully merging this pull request may close these issues.

2 participants