Thread sample weights through RandomForest Python API - #8306
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
RAMitchell
added
improvement
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRandomForest training and scoring now accept sample weights through the accelerator override, Cython bindings, C++ row sampling, and Python estimator APIs. Class-weight handling is added for the classifier, and tests plus sklearn compatibility expectations are updated. ChangesRandomForest weighted training and scoring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuml/cuml/ensemble/randomforestclassifier.py`:
- Around line 262-294: The unsupported class_weight validation in
RandomForestClassifier.fit should run before check_inputs(...), since the
current order can mutate fit-time state via reset=True before raising. Move the
balanced_subsample NotImplementedError check to the start of fit() in
RandomForestClassifier, ahead of input validation and process_class_weight, and
add a test covering the error path to ensure the estimator is not partially
updated on failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2fe3ddbe-5dab-403a-a252-72fdb7ddcd22
📒 Files selected for processing (11)
python/cuml/cuml/accel/_overrides/sklearn/ensemble.pypython/cuml/cuml/dask/ensemble/randomforestclassifier.pypython/cuml/cuml/dask/ensemble/randomforestregressor.pypython/cuml/cuml/ensemble/randomforest_common.pyxpython/cuml/cuml/ensemble/randomforestclassifier.pypython/cuml/cuml/ensemble/randomforestregressor.pypython/cuml/cuml_accel_tests/integration/test_rf_classifier.pypython/cuml/cuml_accel_tests/integration/test_rf_regressor.pypython/cuml/tests/dask/test_dask_random_forest.pypython/cuml/tests/test_random_forest.pypython/cuml/tests/test_sklearn_compatibility.py
There was a problem hiding this comment.
Pull request overview
Adds sample-weighted training and scoring support to cuML’s single-GPU RandomForest Python API, threads weights through the Cython/C++ training path, and introduces class_weight for classification (with tests to validate behavior vs scikit-learn). It also updates sklearn-accelerated and Dask-distributed RF entry points to reflect the new API surface (with Dask explicitly rejecting sample_weight).
Changes:
- Add
sample_weighttoRandomForestClassifier/Regressor.fitand thread it throughrandomforest_common.pyxinto the C++ training call. - Support weighted
score()for classifier (accuracy) and regressor (R²), plusclass_weightfor RF classification ('balanced'and dict). - Add/adjust tests for zero-weight samples, weighted scoring,
min_samples_leafinteractions, and sklearn compatibility check exclusions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/cuml/tests/test_sklearn_compatibility.py | Skips sklearn estimator checks where RF’s binning makes sample-weight equivalence checks invalid. |
| python/cuml/tests/test_random_forest.py | Adds sklearn comparison tests for sample_weight, weighted scoring, min_samples_leaf, and class_weight equivalence. |
| python/cuml/tests/dask/test_dask_random_forest.py | Adds coverage that distributed RF raises for unsupported sample_weight. |
| python/cuml/cuml/ensemble/randomforestregressor.py | Adds sample_weight to fit and weighted R² support in score. |
| python/cuml/cuml/ensemble/randomforestclassifier.py | Adds sample_weight to fit, introduces class_weight support, and weighted accuracy in score. |
| python/cuml/cuml/ensemble/randomforest_common.pyx | Threads sample_weight pointer into the Cython → C++ RF training calls. |
| python/cuml/cuml/dask/ensemble/randomforestregressor.py | Extends distributed regressor fit signature and rejects sample_weight with NotImplementedError. |
| python/cuml/cuml/dask/ensemble/randomforestclassifier.py | Extends distributed classifier fit signature and rejects sample_weight with NotImplementedError. |
| python/cuml/cuml/accel/_overrides/sklearn/ensemble.py | Enables sklearn-accelerated RF proxy to forward sample_weight to the GPU implementation. |
| python/cuml/cuml_accel_tests/integration/test_rf_regressor.py | Adds integration coverage for weighted RF regressor behavior through the accel path. |
| python/cuml/cuml_accel_tests/integration/test_rf_classifier.py | Adds integration coverage for weighted RF classifier behavior through the accel path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
A few nits, but overall this is looking good (assuming tests pass)!
1293fb3 to
f7d280b
Compare
| RandomForestClassifier: { | ||
| "check_sample_weight_equivalence_on_dense_data": ( | ||
| "RandomForest uses quantile-binned splits, so sample weighting is " | ||
| "not equivalent to duplicating rows" |
There was a problem hiding this comment.
Can you comment on this a bit more? The statistical semantics of sample_weight matters, and this is the one sklearn is attempting to standardize (they also have some work to do to get here across all estimators).
Are we approximately equal to this, but off by some amount due to the method used? Or are these weights conceptually different?
There was a problem hiding this comment.
Good that you pointed this out. Sklearn itself xfails this because of boostrapping randomness. However, we should be able to show equivalence to sklearn when bootstrapping=false and we can't. This is because sklearn was not counting zero weight instances towards min_samples and we were. So I will keep the xfail here, but fix this and add another test that checks this specific case.
There was a problem hiding this comment.
We still cannot pass the test because quantisation is not aware of zero weight rows leading to slight differences with sklearn. I think this is not worth dealing with in this PR. In the future we could make quantisation weight aware ideally.
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Two small python nits, but otherwise this LGTM! Thanks for your work here.
Approving on my end, I'll leave it up to you if/how you want to address these.
|
/merge |
Summary
sample_weightsupport to cuML RandomForest classifier/regressorfitscoreclass_weightsupport for RF classification, including"balanced"and explicit class-weight dictionariesclass_weight="balanced_subsample"for nowmin_samples_leaf, and class-weight equivalenceNotes
This relies on the C++ weighted RF training support already merged in the prep work. The current PR keeps cuML's existing
max_samples=1.0behavior and works around sklearn'smax_samples=Nonedefault only in tests where sklearn parameter conversion is needed.Follow-up issue for the
max_samplesdefault/semantics discrepancy: #8305Testing
pre-commit run --files python/cuml/tests/test_random_forest.pypython -m compileall -q python/cuml/tests/test_random_forest.pypytest -q python/cuml/tests/test_random_forest.py -k 'sample_weight or class_weight'