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

Gracefully accept 'n_jobs', a common sklearn parameter, in NearestNeighbors Estimator #4178

Merged
merged 7 commits into from
Sep 9, 2021

Conversation

NV-jpt
Copy link
Contributor

@NV-jpt NV-jpt commented Aug 26, 2021

This pull request partially solves [FEA] #3461.

This quick-fix has been created to enable cuML's NearestNeighbor estimator to gracefully accept sklearns 'n_jobs' parameter as a pass-through.

The purpose of making this quick fix is to allow Imbalanced-Learn samplers to rely on cuML's NearestNeighbor estimator, without producing an error when setting the estimators n_jobs parameter .set_params(**{"n_jobs": self.n_jobs}) 1

@NV-jpt NV-jpt requested a review from a team as a code owner August 26, 2021 18:23
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Aug 26, 2021
@NV-jpt NV-jpt marked this pull request as draft August 27, 2021 21:14
@NV-jpt NV-jpt marked this pull request as ready for review August 30, 2021 16:32
@NV-jpt NV-jpt added 3 - Ready for Review Ready for review by team feature request New feature or request labels Aug 30, 2021
python/cuml/neighbors/nearest_neighbors.pyx Outdated Show resolved Hide resolved
python/cuml/neighbors/nearest_neighbors.pyx Outdated Show resolved Hide resolved
python/cuml/neighbors/nearest_neighbors.pyx Outdated Show resolved Hide resolved
@dantegd dantegd added this to PR-WIP in v21.10 Release via automation Aug 30, 2021
@dantegd dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 3 - Ready for Review Ready for review by team labels Aug 30, 2021
Copy link
Member

@dantegd dantegd left a comment

Choose a reason for hiding this comment

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

Just had 2 very minor comments

python/cuml/neighbors/nearest_neighbors.pyx Outdated Show resolved Hide resolved
python/cuml/neighbors/nearest_neighbors.pyx Outdated Show resolved Hide resolved
v21.10 Release automation moved this from PR-WIP to PR-Needs review Aug 31, 2021
@NV-jpt NV-jpt added 3 - Ready for Review Ready for review by team and removed 4 - Waiting on Author Waiting for author to respond to review labels Sep 1, 2021
@dantegd dantegd added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 8, 2021
@dantegd
Copy link
Member

dantegd commented Sep 8, 2021

@NV-jpt there seems to be just some super minor pep8 issues that the style checker is complaining about:

./python/cuml/neighbors/nearest_neighbors.pyx:284:73: W291 trailing whitespace
./python/cuml/neighbors/nearest_neighbors.pyx:285:72: W291 trailing whitespace
./python/cuml/neighbors/nearest_neighbors.pyx:286:78: W291 trailing whitespace

@beckernick beckernick removed the feature request New feature or request label Sep 8, 2021
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@cae8034). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #4178   +/-   ##
===============================================
  Coverage                ?   85.97%           
===============================================
  Files                   ?      231           
  Lines                   ?    18502           
  Branches                ?        0           
===============================================
  Hits                    ?    15907           
  Misses                  ?     2595           
  Partials                ?        0           
Flag Coverage Δ
dask 47.32% <0.00%> (?)
non-dask 78.57% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cae8034...277b91e. Read the comment docs.

v21.10 Release automation moved this from PR-Needs review to PR-Reviewer approved Sep 9, 2021
@dantegd
Copy link
Member

dantegd commented Sep 9, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 557448e into rapidsai:branch-21.10 Sep 9, 2021
v21.10 Release automation moved this from PR-Reviewer approved to Done Sep 9, 2021
rapids-bot bot pushed a commit that referenced this pull request Nov 15, 2021
… NearestNeighbors Estimator" (#4267)

This pull request partially solves [[FEA] #3461](#3461)

This quick-fix has been created to enable cuML's NearestNeighbor estimator to gracefully accept sklearns 'n_jobs' parameter as a pass-through.

The purpose of making this quick fix is to allow Imbalanced-Learn samplers to rely on cuML's NearestNeighbor estimator, without producing an error when setting the estimators n_jobs parameter .set_params(**{"n_jobs": self.n_jobs})

The[ original PR ](#4178 address this issue was not sufficient, as [`set_params()`](https://github.com/rapidsai/cuml/blob/067344041b1563b19301e2e69240a56605a67997/python/cuml/common/base.pyx#L248) will still raise a ValueError if "n_jobs" is not returned by [`get_param_names()`](https://github.com/rapidsai/cuml/blob/067344041b1563b19301e2e69240a56605a67997/python/cuml/neighbors/nearest_neighbors.pyx#L453)

Authors:
  - https://github.com/NV-jpt
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - William Hicks (https://github.com/wphicks)

URL: #4267
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
…ghbors Estimator (rapidsai#4178)

This pull request partially solves [[FEA] rapidsai#3461](rapidsai#3461).

This quick-fix has been created to enable cuML's NearestNeighbor estimator to gracefully accept sklearns 'n_jobs' parameter as a pass-through.  

The purpose of making this quick fix is to allow Imbalanced-Learn samplers to rely on cuML's NearestNeighbor estimator, without producing an error when setting the estimators n_jobs parameter `.set_params(**{"n_jobs": self.n_jobs})` [1](https://github.com/scikit-learn-contrib/imbalanced-learn/blob/edf6eae2c00f7fa6d76ee381f5b625155061a725/imblearn/over_sampling/_adasyn.py#L112)

Authors:
  - https://github.com/NV-jpt

Approvers:
  - Dante Gama Dessavre (https://github.com/dantegd)

URL: rapidsai#4178
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
… NearestNeighbors Estimator" (rapidsai#4267)

This pull request partially solves [[FEA] rapidsai#3461](rapidsai#3461)

This quick-fix has been created to enable cuML's NearestNeighbor estimator to gracefully accept sklearns 'n_jobs' parameter as a pass-through.

The purpose of making this quick fix is to allow Imbalanced-Learn samplers to rely on cuML's NearestNeighbor estimator, without producing an error when setting the estimators n_jobs parameter .set_params(**{"n_jobs": self.n_jobs})

The[ original PR ](rapidsai#4178 address this issue was not sufficient, as [`set_params()`](https://github.com/rapidsai/cuml/blob/2fee231ac28d982f64c4a746c25be19750812e81/python/cuml/common/base.pyx#L248) will still raise a ValueError if "n_jobs" is not returned by [`get_param_names()`](https://github.com/rapidsai/cuml/blob/2fee231ac28d982f64c4a746c25be19750812e81/python/cuml/neighbors/nearest_neighbors.pyx#L453)

Authors:
  - https://github.com/NV-jpt
  - Dante Gama Dessavre (https://github.com/dantegd)

Approvers:
  - William Hicks (https://github.com/wphicks)

URL: rapidsai#4267
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants