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

[FEA] Merge MNMG NN and MNMG NN Cl&Re #3197

Closed
viclafargue opened this issue Nov 30, 2020 · 2 comments
Closed

[FEA] Merge MNMG NN and MNMG NN Cl&Re #3197

viclafargue opened this issue Nov 30, 2020 · 2 comments
Assignees
@viclafargue
Labels
CUDA / C++ CUDA issue Cython / Python Cython or Python issue Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. feature request New feature or request inactive-30d Multi-GPU Issues & PRs related to multi-GPU functionality

Comments

@viclafargue
Copy link
Contributor

Is your feature request related to a problem? Please describe.
MNMG NN and MNMG NN Cl&Re currently have largely separate code. For increased stability and maintainability, it would be interesting to share some portion of code that could be common to these two algorithms. Along with that change, references to indices and distances could also be removed from MNMG NN Cl&Re as long as it doesn't impede code refactoring.
@viclafargue viclafargue added ? - Needs Triage Need team to review and classify feature request New feature or request labels Nov 30, 2020
@viclafargue viclafargue self-assigned this Nov 30, 2020
@viclafargue viclafargue added CUDA / C++ CUDA issue Cython / Python Cython or Python issue Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. Multi-GPU Issues & PRs related to multi-GPU functionality and removed ? - Needs Triage Need team to review and classify labels Nov 30, 2020
@viclafargue viclafargue mentioned this issue Jan 19, 2021
Merged
rapids-bot bot pushed a commit that referenced this issue Feb 2, 2021
@viclafargue
MNMG KNN consolidation (#3307)
fa2371a 
Answers #3197

Currently, MNMG KNN and MNMG KNN Cl&Re use separate code. This PR intends to refactor the code on both the Python and C++ sides to use common code as much as possible. In addition, the functions in the C++ code currently have a very large number of arguments, making them unreadable to newcomers. This PR will work on reducing their number with the use of parameter structures like it's done elsewhere in cuML's codebase.

Authors:
  - Victor Lafargue (@viclafargue)

Approvers:
  - Corey J. Nolet (@cjnolet)
  - Dante Gama Dessavre (@dantegd)

URL: #3307
@github-actions
Copy link

This issue has been marked stale due to no recent activity in the past 30d. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be marked rotten if there is no activity in the next 60d.

@viclafargue
Copy link
Contributor Author

Closed by #3307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@viclafargue viclafargue

Labels
CUDA / C++ CUDA issue Cython / Python Cython or Python issue Dask / cuml.dask Issue/PR related to Python level dask or cuml.dask features. feature request New feature or request inactive-30d Multi-GPU Issues & PRs related to multi-GPU functionality
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@viclafargue