[HOTFIX] Fix distance metrics L2/cosine/correlation when X & Y are same buffer but with different shape and add unit test for such case. #1571
Conversation
…same buffer but with different shape, also add unit test support for them
| @@ -137,7 +137,7 @@ void distance_impl(raft::resources const& handle, | |||
| AccT* y_norm = workspace; | |||
| AccT* sq_x_norm = workspace; | |||
| AccT* sq_y_norm = workspace; | |||
| if (x != y) { | |||
| if ((x != y) || ((x == y) && (m != n))) { | |||
There was a problem hiding this comment.
This conditional is getting a little complicated and I see it in multiple places in this PR. Can we create a helper function that can give this a name and be reused throughout the code? It would make it a lot easier to read and maintain.
There was a problem hiding this comment.
have removed this conditional.
|
@mdoijade do you still think we can get this fix into 23.06? It looks like there's some c++ test failures |
I believe the failure is related to fp arithmetic accuracy issues in L2 distances for X == Y input cases. I don't see a bug as such I will fix the tolerance accordingly and push it soon. |
…ol until reduction accuracy issue is resolved
There was a problem hiding this comment.
Thanks @mdoijade for the fix, it looks good to me.
We shall return to this in follow up pr, to fix potential problems at two more distance types:
cjnolet
changed the title
-- This is how tiled_brute_force_knn may use pairwise distance API hence assuming when X == Y the buffer has same shape is incorrect.