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

Fea 2208 kmeans use specializations #760

Merged

Conversation

cjnolet
Copy link
Member

@cjnolet cjnolet commented Jul 27, 2022

No description provided.

@github-actions github-actions bot added the cpp label Jul 27, 2022
@cjnolet cjnolet marked this pull request as ready for review July 27, 2022 18:07
@cjnolet cjnolet requested a review from a team as a code owner July 27, 2022 18:07
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jul 27, 2022
@cjnolet cjnolet self-assigned this Jul 28, 2022
@cjnolet cjnolet changed the base branch from branch-22.08 to branch-22.10 August 24, 2022 19:08
@cjnolet
Copy link
Member Author

cjnolet commented Sep 6, 2022

@Nyrio just FYI, I did realize after some investigation that this doesn't fix the horrid build time of k-means because the standard distance instantiations seem to be getting ignored- telling me k-means must be using a template combination that we're not instantiating. I haven't investigated exactly which ones it's using yet, though.

@Nyrio
Copy link
Contributor

Nyrio commented Sep 6, 2022

@cjnolet I've compiled the kmeans test with your changes and then run nm and this is what I found:

  • 28/32 of the raft:distance::detail::distance are marked as "U" i.e undefined in this file (correctly identified as extern)
  • The 4 remaining correspond to a metric that hasn't been added to the specializations, as I pointed in a comment on my PR
  • 20 instances of fusedL2NNkernel are recompiled every time.

I think your PR is working. Horrid compilation times are due to fusedL2NN, which is why I made a specialization for it in #795

@cjnolet
Copy link
Member Author

cjnolet commented Sep 6, 2022

Perfect and thanks for investigating this! If you'd like, we can get this PR merged ASAP and then add the specializations for the missing metrics in your PR. Or I can add them here, whichever you prefer.

@Nyrio
Copy link
Contributor

Nyrio commented Sep 6, 2022

I think we can merge this one, and I'll add the rest to my PR.

@cjnolet
Copy link
Member Author

cjnolet commented Sep 6, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 700bb1e into rapidsai:branch-22.10 Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants