[REVIEW] RF param initialization cython and C++ layer cleanup #3358

venkywonka · 2021-01-11T06:28:13Z

This PR partially solves the issue raised here.
Removes unused DecisionTreeParams struct in randomforest_shared.pxd.
Unifies the different APIs (namely set_rf_params, set_all_rf_params, set_rf_class_obj) into a single point of parameter initialization (as set_rf_params) in the C++ layer; and propagating the changes.

* deleting unused DecisionTreeParams class in cython layer * unifying the APIs that set params to randomforest class to create single point of entry

…nh-ext-rf-param-init-api-change

hcho3 · 2021-01-11T07:07:50Z

Will try to review this week.

venkywonka · 2021-01-11T08:23:59Z

Will try to review this week.

thanks @hcho3 😃

teju85 · 2021-01-21T09:27:35Z

@hcho3 did you get a chance to review this?

cpp/test/sg/rf_accuracy_test.cu

hcho3

LGTM. I have some minor comments.

cpp/src/randomforest/randomforest.cu

… enh-ext-rf-param-init-api-change

* pruned comments and a redundant line

venkywonka · 2021-02-01T03:32:11Z

rerun tests

JohnZed · 2021-02-03T00:00:46Z

Oof! Looks like this ended up with some merge conflicts due to other RF merges. @venkywonka would you mind taking a look? We should still be able to get this into 0.18

… enh-ext-rf-param-init-api-change

venkywonka · 2021-02-03T07:47:23Z

Oof! Looks like this ended up with some merge conflicts due to other RF merges. @venkywonka would you mind taking a look? We should still be able to get this into 0.18

sure @JohnZed , done! ✌🏾

… enh-ext-rf-param-init-api-change

venkywonka · 2021-02-10T03:17:56Z

rerun tests

vinaydes · 2021-02-15T01:54:21Z

@venkywonka Can you bring your branch forward to 0.19? Also revert changes done to CHANGELOG.md. Thanks.

… enh-ext-rf-param-init-api-change

…om/venkywonka/cuml into enh-ext-rf-param-init-api-change

venkywonka · 2021-02-15T03:16:45Z

@venkywonka Can you bring your branch forward to 0.19? Also revert changes done to CHANGELOG.md. Thanks.

done vinay

venkywonka · 2021-02-17T04:35:21Z

rerun tests

… enh-ext-rf-param-init-api-change

JohnZed · 2021-03-01T20:01:51Z

rerun tests

…aram-init-api-change

codecov-io · 2021-03-16T10:16:53Z

Codecov Report

Merging #3358 (b8f7609) into branch-0.19 (f5d86b9) will decrease coverage by 27.94%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           branch-0.19    #3358       +/-   ##
================================================
- Coverage        72.91%   44.96%   -27.95%     
================================================
  Files              214      223        +9     
  Lines            16856    16932       +76     
================================================
- Hits             12290     7613     -4677     
- Misses            4566     9319     +4753

Flag	Coverage Δ
dask	`44.96% <100.00%> (?)`
non-dask	`?`

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

Impacted Files	Coverage Δ
python/cuml/ensemble/randomforestclassifier.pyx	`58.92% <100.00%> (-14.48%)`	⬇️
python/cuml/ensemble/randomforestregressor.pyx	`67.34% <100.00%> (-3.27%)`	⬇️
python/cuml/_thirdparty/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
python/cuml/thirdparty_adapters/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
python/cuml/_thirdparty/sklearn/exceptions.py	`0.00% <0.00%> (-100.00%)`	⬇️
python/cuml/experimental/explainer/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
python/cuml/_thirdparty/sklearn/utils/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
python/cuml/experimental/linear_model/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
python/cuml/experimental/preprocessing/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
...ython/cuml/experimental/hyperopt_utils/__init__.py	`0.00% <0.00%> (-100.00%)`	⬇️
... and 157 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 f5d86b9...b8f7609. Read the comment docs.

teju85

Changes LGTM.

teju85 · 2021-03-17T03:35:04Z

@gpucibot merge

venkywonka added 2 commits January 11, 2021 11:29

pruning RF param init APIs 👇

041a48b

* deleting unused DecisionTreeParams class in cython layer * unifying the APIs that set params to randomforest class to create single point of entry

Merge branch 'branch-0.18' of https://github.com/rapidsai/cuml into e…

40abcff

…nh-ext-rf-param-init-api-change

venkywonka requested review from a team as code owners January 11, 2021 06:28

venkywonka added CUDA / C++ CUDA issue Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Jan 11, 2021

FIX clang-format fixes

493efe1

hcho3 self-requested a review January 11, 2021 07:07

FIX flake8 style

711a236

hcho3 reviewed Jan 22, 2021

View reviewed changes

cpp/test/sg/rf_accuracy_test.cu Outdated Show resolved Hide resolved

hcho3 requested changes Jan 22, 2021

View reviewed changes

cpp/src/randomforest/randomforest.cu Outdated Show resolved Hide resolved

venkywonka added 2 commits January 23, 2021 10:52

🔀 Merge branch 'branch-0.18' of https://github.com/rapidsai/cuml into…

44dba50

… enh-ext-rf-param-init-api-change

♻ review changes👇

3882049

* pruned comments and a redundant line

github-actions bot added the libcuml label Jan 23, 2021

hcho3 approved these changes Jan 23, 2021

View reviewed changes

📄 update copyright

120e175

hcho3 approved these changes Jan 25, 2021

View reviewed changes

JohnZed approved these changes Jan 26, 2021

View reviewed changes

venkywonka added 2 commits February 3, 2021 12:50

🔀 Merge branch 'branch-0.18' of https://github.com/rapidsai/cuml into…

b346ca9

… enh-ext-rf-param-init-api-change

FIX clang-format fixes

7f374f4

🔀 Merge branch 'branch-0.18' of https://github.com/rapidsai/cuml into…

032c892

… enh-ext-rf-param-init-api-change

JohnZed changed the base branch from branch-0.18 to branch-0.19 February 10, 2021 05:57

JohnZed requested a review from a team as a code owner February 10, 2021 05:57

Merge branch 'branch-0.19' into enh-ext-rf-param-init-api-change

0fe3d16

github-actions bot added the CMake label Feb 10, 2021

venkywonka added 2 commits February 15, 2021 08:39

🔀 Merge branch 'branch-0.19' of https://github.com/rapidsai/cuml into…

9b5f108

… enh-ext-rf-param-init-api-change

🔀 Merge branch 'enh-ext-rf-param-init-api-change' of https://github.c…

286d741

…om/venkywonka/cuml into enh-ext-rf-param-init-api-change

github-actions bot removed the CMake label Feb 15, 2021

🔀 Merge branch 'branch-0.19' of https://github.com/rapidsai/cuml into…

3d6a960

… enh-ext-rf-param-init-api-change

Merge remote-tracking branch 'upstream/branch-0.19' into enh-ext-rf-p…

b8f7609

…aram-init-api-change

github-actions bot added the CUDA/C++ label Mar 16, 2021

venkywonka changed the title ~~RF param initialization cython and C++ layer cleanup~~ [REVIEW] RF param initialization cython and C++ layer cleanup Mar 17, 2021

teju85 approved these changes Mar 17, 2021

View reviewed changes

rapids-bot bot merged commit 14bd6c1 into rapidsai:branch-0.19 Mar 17, 2021

venkywonka mentioned this pull request Jun 21, 2021

[TRACKER] Clearing technical debt of RF/DT code #3999

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] RF param initialization cython and C++ layer cleanup #3358

[REVIEW] RF param initialization cython and C++ layer cleanup #3358

venkywonka commented Jan 11, 2021

hcho3 commented Jan 11, 2021

venkywonka commented Jan 11, 2021

teju85 commented Jan 21, 2021

hcho3 left a comment

venkywonka commented Feb 1, 2021

JohnZed commented Feb 3, 2021

venkywonka commented Feb 3, 2021

venkywonka commented Feb 10, 2021

vinaydes commented Feb 15, 2021

venkywonka commented Feb 15, 2021

venkywonka commented Feb 17, 2021

JohnZed commented Mar 1, 2021

codecov-io commented Mar 16, 2021

teju85 left a comment

teju85 commented Mar 17, 2021

[REVIEW] RF param initialization cython and C++ layer cleanup #3358

[REVIEW] RF param initialization cython and C++ layer cleanup #3358

Conversation

venkywonka commented Jan 11, 2021

hcho3 commented Jan 11, 2021

venkywonka commented Jan 11, 2021

teju85 commented Jan 21, 2021

hcho3 left a comment

Choose a reason for hiding this comment

venkywonka commented Feb 1, 2021

JohnZed commented Feb 3, 2021

venkywonka commented Feb 3, 2021

venkywonka commented Feb 10, 2021

vinaydes commented Feb 15, 2021

venkywonka commented Feb 15, 2021

venkywonka commented Feb 17, 2021

JohnZed commented Mar 1, 2021

codecov-io commented Mar 16, 2021

Codecov Report

teju85 left a comment

Choose a reason for hiding this comment

teju85 commented Mar 17, 2021