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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Modify default value for rowMajorIndex and rowMajorQuery in bf-knn #173

Merged
merged 2 commits into from
Mar 24, 2021
Merged

[REVIEW] Modify default value for rowMajorIndex and rowMajorQuery in bf-knn #173

merged 2 commits into from
Mar 24, 2021

Conversation

viclafargue
Copy link
Contributor

This PR modifies the default value for rowMajorIndex and rowMajorQuery parameters in brute_force_knn.

@github-actions github-actions bot added the cpp label Mar 18, 2021
@viclafargue viclafargue added 3 - Ready for Review bug Something isn't working non-breaking Non-breaking change labels Mar 18, 2021
@cjnolet
Copy link
Member

cjnolet commented Mar 22, 2021

@hlinsen, this PR modifies the default values for the rowMajorIndex and rowMajorQuery arguments. There was also a recent PR that removed the expanded argument from brute_force_knn. I wanted to make sure you are aware of this so cugraph can be updated and we can avoid any unexpected problems during the coming release.

also cc @JohnZed

@hlinsen
Copy link
Contributor

hlinsen commented Mar 22, 2021

@hlinsen, this PR modifies the default values for the rowMajorIndex and rowMajorQuery arguments. There was also a recent PR that removed the expanded argument from brute_force_knn. I wanted to make sure you are aware of this so cugraph can be updated and we can avoid any unexpected problems during the coming release.

also cc @JohnZed

Sounds good. This shouldn't cause issues, if there are I have a PR coming soon with TSP fixes so I'll make the necessary changes there.

@cjnolet
Copy link
Member

cjnolet commented Mar 22, 2021

@viclafargue, I've opened #178, which should fix the CUDA11 failing SLHC test. I'm still working to find the cause / fix for the CUDA10.1/2 hang issue.

cjnolet
cjnolet approved these changes Mar 23, 2021
View reviewed changes
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.

LGTM

@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

@gpucibot merge

@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

rerun tests

@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

@viclafargue, the PR to fix the linkage test went through last night. Just merged branch-0.19 into your branch in hopes we might finally be able to get this PR merged.

@cjnolet
Copy link
Member

cjnolet commented Mar 24, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e21d9dd into rapidsai:branch-0.19 Mar 24, 2021
dantegd pushed a commit to dantegd/raft that referenced this pull request Jul 23, 2024
@benfred
Allow serialization on streams (rapidsai#173)
38b3147 
This change allows serializing to a std::ostream and deserializaing from a std::istream. This also fixes some minor docstring issues in the C++ serialization api's.

Authors:
  - Ben Frederickson (https://github.com/benfred)

Approvers:
  - Divye Gala (https://github.com/divyegala)

URL: rapidsai/cuvs#173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjnolet cjnolet cjnolet approved these changes

Assignees
No one assigned
Labels
5 - Ready to Merge bug Something isn't working cpp non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@viclafargue @cjnolet @hlinsen