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

Remove old RF backend #3868

Merged
merged 13 commits into from
Jun 17, 2021
Merged

Remove old RF backend #3868

merged 13 commits into from
Jun 17, 2021

Conversation

RAMitchell
Copy link
Contributor

Removes split_algo, use_experimental_backend parameters. Random forest classification and regression default n_bins is set to 128.

@RAMitchell RAMitchell requested review from a team as code owners May 17, 2021 06:08
@github-actions github-actions bot added CUDA/C++ Cython / Python Cython or Python issue labels May 17, 2021
@RAMitchell RAMitchell changed the base branch from branch-21.06 to branch-21.08 May 25, 2021 22:28
@teju85 teju85 added breaking Breaking change improvement Improvement / enhancement to an existing function labels Jun 4, 2021
@teju85
Copy link
Member

teju85 commented Jun 4, 2021

@RAMitchell did you get a chance to resolve the conflicts? I think we should push for complete removal of the old backend in 21.08 release (preferably during its early development stages).

@JohnZed or @dantegd , any objections to getting this merged in the early 21.08 stages?

@RAMitchell
Copy link
Contributor Author

I can probably get to it Tuesday my time.

@RAMitchell RAMitchell changed the title [WIP] Remove old RF backend Remove old RF backend Jun 8, 2021
teju85
teju85 approved these changes Jun 14, 2021
View reviewed changes
Copy link
Member

@teju85 teju85 left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Only have left a few minor nitpicks. Thus, pre-approving this.

python/cuml/ensemble/randomforest_common.pyx Outdated Show resolved Hide resolved
python/cuml/ensemble/randomforest_common.pyx Outdated Show resolved Hide resolved
python/cuml/ensemble/randomforestclassifier.pyx Outdated Show resolved Hide resolved
python/cuml/ensemble/randomforestregressor.pyx Outdated Show resolved Hide resolved
vinaydes
vinaydes approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@vinaydes vinaydes left a comment

Choose a reason for hiding this comment

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

LGTM

@hcho3
Copy link
Contributor

hcho3 commented Jun 14, 2021

Rerun tests

@dantegd
Copy link
Member

dantegd commented Jun 16, 2021

rerun tests

dantegd
dantegd approved these changes Jun 16, 2021
View reviewed changes
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.

Python codeowner approval

@teju85
Copy link
Member

teju85 commented Jun 16, 2021

@cjnolet or @divyegala any ideas why hdbscan related tests seem to be failing for this PR!?

@RAMitchell
Copy link
Contributor Author

rerun tests

@codecov-commenter
Copy link

Codecov Report

Merging #3868 (38a4f1b) into branch-21.08 (c35680f) will increase coverage by 0.33%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.08    #3868      +/-   ##
================================================
+ Coverage         85.23%   85.57%   +0.33%     
================================================
  Files               230      230              
  Lines             17967    18272     +305     
================================================
+ Hits              15315    15636     +321     
+ Misses             2652     2636      -16
Flag Coverage Δ
dask 48.18% <100.00%> (+0.62%) ⬆️
non-dask 77.98% <100.00%> (+0.44%) ⬆️

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

Impacted Files Coverage Δ
...ython/cuml/dask/ensemble/randomforestclassifier.py 61.36% <ø> (+0.89%) ⬆️
python/cuml/dask/ensemble/randomforestregressor.py 88.70% <ø> (+0.37%) ⬆️
python/cuml/ensemble/randomforestclassifier.pyx 83.27% <ø> (-0.34%) ⬇️
python/cuml/ensemble/randomforestregressor.pyx 84.64% <ø> (-0.26%) ⬇️
python/cuml/ensemble/randomforest_common.pyx 94.17% <100.00%> (+0.39%) ⬆️
python/cuml/dask/solvers/cd.py 100.00% <0.00%> (ø)
python/cuml/common/numba_utils.py 0.00% <0.00%> (ø)
python/cuml/decomposition/base_mg.pyx 100.00% <0.00%> (ø)
python/cuml/benchmark/nvtx_benchmark.py 0.00% <0.00%> (ø)
python/cuml/internals/global_settings.py 100.00% <0.00%> (ø)
... and 41 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 c35680f...38a4f1b. Read the comment docs.

@teju85
Copy link
Member

teju85 commented Jun 17, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1fcad06 into rapidsai:branch-21.08 Jun 17, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
@RAMitchell
Remove old RF backend (rapidsai#3868)
3c5ab20 
Removes split_algo, use_experimental_backend parameters. Random forest classification and regression default n_bins is set to 128.

Authors:
  - Rory Mitchell (https://github.com/RAMitchell)
  - Venkat (https://github.com/venkywonka)

Approvers:
  - Thejaswi. N. S (https://github.com/teju85)
  - Vinay Deshpande (https://github.com/vinaydes)
  - Philip Hyunsu Cho (https://github.com/hcho3)
  - Dante Gama Dessavre (https://github.com/dantegd)

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

@venkywonka venkywonka venkywonka left review comments

@vinaydes vinaydes vinaydes approved these changes

@hcho3 hcho3 hcho3 approved these changes

@dantegd dantegd dantegd approved these changes

@teju85 teju85 teju85 approved these changes

Assignees
No one assigned
Labels
breaking Breaking change CUDA/C++ Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@RAMitchell @teju85 @hcho3 @dantegd @codecov-commenter @vinaydes @venkywonka