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

Update of "Gracefully accept 'n_jobs', a common sklearn parameter, in NearestNeighbors Estimator" #4267

Merged
merged 13 commits into from
Nov 15, 2021

Conversation

NV-jpt
Copy link
Contributor

@NV-jpt NV-jpt commented Oct 4, 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})

The original PR to address this issue was not sufficient, as set_params() will still raise a ValueError if "n_jobs" is not returned by get_param_names()

@NV-jpt NV-jpt requested a review from a team as a code owner October 4, 2021 19:34
@github-actions github-actions bot added the Cython / Python Cython or Python issue label Oct 4, 2021
@cjnolet cjnolet added this to PR-WIP in v21.12 Release via automation Oct 4, 2021
@cjnolet cjnolet added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 4, 2021
cjnolet
cjnolet previously requested changes Oct 4, 2021
python/cuml/neighbors/nearest_neighbors.pyx Show resolved Hide resolved
v21.12 Release automation moved this from PR-WIP to PR-Needs review Oct 4, 2021
@dantegd dantegd added the 4 - Waiting on Reviewer Waiting for reviewer to review or respond label Oct 6, 2021
@cjnolet cjnolet added 4 - Waiting on Author Waiting for author to respond to review and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Oct 8, 2021
@NV-jpt NV-jpt added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 4 - Waiting on Author Waiting for author to respond to review labels Oct 27, 2021
Thank you, wphicks!

Co-authored-by: William Hicks <wphicks@users.noreply.github.com>
@dantegd dantegd dismissed cjnolet’s stale review November 12, 2021 17:50

Change has been addressed

v21.12 Release automation moved this from PR-Needs review to PR-Reviewer approved Nov 12, 2021
@NV-jpt
Copy link
Contributor Author

NV-jpt commented Nov 12, 2021

rerun tests

@wphicks
Copy link
Contributor

wphicks commented Nov 12, 2021

Minor style issue; should be easy to take care of

@codecov-commenter
Copy link

Codecov Report

Merging #4267 (ea1c40c) into branch-21.12 (835a9ae) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #4267      +/-   ##
================================================
- Coverage         86.06%   86.03%   -0.04%     
================================================
  Files               231      231              
  Lines             18691    18751      +60     
================================================
+ Hits              16087    16132      +45     
- Misses             2604     2619      +15     
Flag Coverage Δ
dask 47.02% <0.00%> (+<0.01%) ⬆️
non-dask 78.74% <100.00%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
python/cuml/neighbors/nearest_neighbors.pyx 92.41% <100.00%> (+0.13%) ⬆️
...ython/cuml/_thirdparty/sklearn/utils/validation.py 66.66% <0.00%> (-3.93%) ⬇️
python/cuml/tsa/batched_lbfgs.py 70.65% <0.00%> (-3.27%) ⬇️
python/cuml/metrics/trustworthiness.pyx 87.50% <0.00%> (-2.75%) ⬇️
python/cuml/tsa/seasonality.pyx 32.00% <0.00%> (-2.62%) ⬇️
...rty/sklearn/preprocessing/_function_transformer.py 97.61% <0.00%> (-2.39%) ⬇️
python/cuml/manifold/t_sne.pyx 76.77% <0.00%> (-0.69%) ⬇️
python/cuml/internals/api_decorators.py 93.67% <0.00%> (-0.58%) ⬇️
python/cuml/svm/svm_base.pyx 95.95% <0.00%> (-0.35%) ⬇️
python/cuml/tsa/arima.pyx 81.20% <0.00%> (-0.26%) ⬇️
... and 32 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 835a9ae...ea1c40c. Read the comment docs.

@dantegd
Copy link
Member

dantegd commented Nov 15, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b5f119e into rapidsai:branch-21.12 Nov 15, 2021
v21.12 Release automation moved this from PR-Reviewer approved to Done Nov 15, 2021
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
4 - Waiting on Reviewer Waiting for reviewer to review or respond 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

5 participants