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

Fix RF broadcast feature #3833

Merged

Conversation

viclafargue
Copy link
Contributor

Answers #3820.

This PR fixes the broadcast feature of the Random Forest estimator. The weights used by the reduction step were generated incorrectly. Indeed, the right values are to be deducted, for each chunk to be predicted, by the number estimators trained by the specific worker holding that chunk. The values wrongly used previously were the number of estimators held by each worker in the order of their construction.

@viclafargue viclafargue requested a review from a team as a code owner May 6, 2021 12:12
@viclafargue viclafargue added 3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change labels May 6, 2021
@github-actions github-actions bot added the Cython / Python Cython or Python issue label May 6, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #3833 (f8d198c) into branch-0.20 (46174b7) will increase coverage by 0.06%.
The diff coverage is 82.47%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20    #3833      +/-   ##
===============================================
+ Coverage        85.96%   86.03%   +0.06%     
===============================================
  Files              225      225              
  Lines            16986    17121     +135     
===============================================
+ Hits             14602    14730     +128     
- Misses            2384     2391       +7     
Flag Coverage Δ
dask 49.30% <28.86%> (+0.33%) ⬆️
non-dask 77.91% <74.22%> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
python/cuml/common/memory_utils.py 79.26% <ø> (+0.51%) ⬆️
python/cuml/dask/common/utils.py 43.68% <0.00%> (ø)
python/cuml/ensemble/randomforestclassifier.pyx 83.61% <ø> (ø)
python/cuml/linear_model/logistic_regression.pyx 89.21% <ø> (ø)
python/cuml/neighbors/nearest_neighbors.pyx 93.11% <ø> (-0.03%) ⬇️
python/cuml/tsa/auto_arima.pyx 57.48% <0.00%> (-0.53%) ⬇️
python/cuml/manifold/t_sne.pyx 78.34% <76.31%> (-1.00%) ⬇️
python/cuml/dask/common/dask_arr_utils.py 95.83% <100.00%> (+0.05%) ⬆️
python/cuml/dask/ensemble/base.py 84.35% <100.00%> (+0.44%) ⬆️
python/cuml/fil/fil.pyx 92.55% <100.00%> (-0.31%) ⬇️
... and 45 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 908b2c4...f8d198c. Read the comment docs.

@viclafargue viclafargue changed the title [REVIEW] Fix RF broadcast feature Fix RF broadcast feature May 10, 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.

lgtm

@dantegd
Copy link
Member

dantegd commented May 10, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 1ea479b into rapidsai:branch-0.20 May 10, 2021
vimarsh6739 pushed a commit to vimarsh6739/cuml that referenced this pull request Oct 9, 2023
Answers rapidsai#3820.

This PR fixes the broadcast feature of the Random Forest estimator. The weights used by the reduction step were generated incorrectly. Indeed, the right values are to be deducted, for each chunk to be predicted, by the number estimators trained by the specific worker holding that chunk. The values wrongly used previously were the number of estimators held by each worker in the order of their construction.

Authors:
  - Victor Lafargue (https://github.com/viclafargue)

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

URL: rapidsai#3833
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 bug Something isn't working Cython / Python Cython or Python issue non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants