RF: add class_weight to RandomForestClassifier - #8188
Closed
switch527 wants to merge 2 commits into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (30)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (26)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR implements sample_weight and class_weight support for RandomForest by threading weighted training through decision-tree kernels, objectives, and estimator APIs. Per-sample weights are accumulated in CUDA histograms, gain computations incorporate weighted totals, and class_weight='balanced_subsample' triggers per-tree balanced-weight recomputation. ChangesRandomForest weighted training
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/include/cuml/ensemble/randomforest.hpp (1)
150-162: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMissing Doxygen documentation for new public API parameters.
The new
sample_weight,class_weight_mode, andclass_weight_arrayparameters lack documentation. Public API functions should have Doxygen comments describing parameter semantics, valid values, and ownership expectations.📝 Suggested documentation additions
+/** + * `@brief` Train a random forest classifier. + * ...existing params... + * `@param`[in] sample_weight Optional per-sample weights (device pointer, length n_rows). + * Pass nullptr for uniform weights. + * `@param`[in] class_weight_mode Class weight mode: 0=NONE, 1=BALANCED_SUBSAMPLE. + * `@param`[in] class_weight_array Optional per-class weight array (device pointer, length n_unique_labels). + * Required when class_weight_mode is NONE and custom class weights are desired. + */ void fit(const raft::handle_t& user_handle, RandomForestClassifierF* forest,As per coding guidelines: "Provide Doxygen documentation for all public functions."
Also applies to: 163-175
🤖 Prompt for 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. In `@cpp/include/cuml/ensemble/randomforest.hpp` around lines 150 - 162, Add Doxygen comments to the public fit overload(s) for RandomForestClassifierF (and the corresponding double/other overload) documenting the new parameters: describe sample_weight (per-sample importance, length n_rows, nullable and ownership/ownership: caller-owned), class_weight_mode (enumerate valid values and behavior for 0/1/... meaning e.g., none/auto/custom), and class_weight_array (when used, length n_unique_labels, ordering/mapping to label indices, nullable and ownership). Also update the brief function description to mention weighted training support and include `@param` tags for sample_weight, class_weight_mode, and class_weight_array consistent with existing Doxygen style used for this header.
🧹 Nitpick comments (3)
cpp/bench/sg/rf_regressor.cu (1)
2-2: 💤 Low valueUpdate copyright year for consistency.
The classifier benchmark was updated to 2026 (cpp/bench/sg/rf_classifier.cu:2), but this file remains at 2024. Consider updating for consistency.
📅 Proposed fix
- * SPDX-FileCopyrightText: Copyright (c) 2019-2024, NVIDIA CORPORATION. + * SPDX-FileCopyrightText: Copyright (c) 2019-2026, NVIDIA CORPORATION.🤖 Prompt for 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. In `@cpp/bench/sg/rf_regressor.cu` at line 2, Update the copyright header in rf_regressor.cu to match the other benchmark file by changing the year range from "2019-2024" to "2019-2026"; locate the top-of-file SPDX/ copyright comment in rf_regressor.cu and edit the year span so it is consistent with rf_classifier.cu.cpp/include/cuml/ensemble/randomforest.hpp (1)
29-32: 💤 Low valueEnum comment references internal implementation file.
The comment mentions
per_tree_weights.cuhwhich is an implementation detail not visible to API consumers. Consider documenting the formula or behavior directly here, or referencing user-facing documentation instead.🤖 Prompt for 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. In `@cpp/include/cuml/ensemble/randomforest.hpp` around lines 29 - 32, The comment on the ClassWeightMode enum currently references an internal file per_tree_weights.cuh; replace that implementation-detail reference with a user-facing description: for ClassWeightMode::NONE state that sample_weight is passed through unchanged, and for ClassWeightMode::BALANCED_SUBSAMPLE explain concisely that class weights are recomputed per tree from each bootstrap sample as the reciprocal of class frequencies (i.e., weight = total_samples / (n_classes * class_count_in_bootstrap)) and note parity with scikit-learn behavior or link to external user documentation instead of the .cuh file; update the enum comment accordingly.cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh (1)
263-294: 💤 Low valueDead code in else branch.
The
elsebranch at lines 280-282 is unreachable because thestatic_assertat line 265 guaranteeskIsClassifier || kIsRegressor. This dead code adds confusion without providing any fallback benefit.♻️ Suggested cleanup
if constexpr (kIsClassifier) { shared_unweighted = alignPointer<int>(shared_histogram + shared_histogram_len); shared_quantiles = alignPointer<DataT>(shared_unweighted + n_bins); } else if constexpr (kIsRegressor) { shared_weighted_count = alignPointer<double>(shared_histogram + shared_histogram_len); shared_quantiles = alignPointer<DataT>(shared_weighted_count + n_bins); - } else { - shared_quantiles = alignPointer<DataT>(shared_histogram + shared_histogram_len); }🤖 Prompt for 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. In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh` around lines 263 - 294, The else branch assigning shared_quantiles is dead due to the static_assert(kIsClassifier || kIsRegressor) in computeSplitKernel; remove the unreachable else block (the branch that does shared_quantiles = alignPointer<DataT>(shared_histogram + shared_histogram_len)) and keep the existing assignments inside the kIsClassifier and kIsRegressor if constexpr branches (or, alternatively, move a single shared_quantiles assignment after those two constexpr branches if you prefer one place of initialization).
🤖 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 `@cpp/tests/sg/rf_test.cu`:
- Around line 704-711: The current assertions only check sizes/counters
(leaf_counter, depth_counter, sparsetree.size(), vector_leaf.size()) but not the
actual sparsetree/node contents; update the test to iterate each tree in
forest_null/forest_ones and assert deep equality of sparsetree elements (compare
corresponding node fields such as split_feature, split_threshold, left/right
indices, and any other node payload) and also compare vector_leaf
element-by-element to ensure byte-identical node structure; apply the same
deep-comparison approach to the other analogous test blocks (the similar checks
around the other occurrences of forest_null/forest_ones).
---
Outside diff comments:
In `@cpp/include/cuml/ensemble/randomforest.hpp`:
- Around line 150-162: Add Doxygen comments to the public fit overload(s) for
RandomForestClassifierF (and the corresponding double/other overload)
documenting the new parameters: describe sample_weight (per-sample importance,
length n_rows, nullable and ownership/ownership: caller-owned),
class_weight_mode (enumerate valid values and behavior for 0/1/... meaning e.g.,
none/auto/custom), and class_weight_array (when used, length n_unique_labels,
ordering/mapping to label indices, nullable and ownership). Also update the
brief function description to mention weighted training support and include
`@param` tags for sample_weight, class_weight_mode, and class_weight_array
consistent with existing Doxygen style used for this header.
---
Nitpick comments:
In `@cpp/bench/sg/rf_regressor.cu`:
- Line 2: Update the copyright header in rf_regressor.cu to match the other
benchmark file by changing the year range from "2019-2024" to "2019-2026";
locate the top-of-file SPDX/ copyright comment in rf_regressor.cu and edit the
year span so it is consistent with rf_classifier.cu.
In `@cpp/include/cuml/ensemble/randomforest.hpp`:
- Around line 29-32: The comment on the ClassWeightMode enum currently
references an internal file per_tree_weights.cuh; replace that
implementation-detail reference with a user-facing description: for
ClassWeightMode::NONE state that sample_weight is passed through unchanged, and
for ClassWeightMode::BALANCED_SUBSAMPLE explain concisely that class weights are
recomputed per tree from each bootstrap sample as the reciprocal of class
frequencies (i.e., weight = total_samples / (n_classes *
class_count_in_bootstrap)) and note parity with scikit-learn behavior or link to
external user documentation instead of the .cuh file; update the enum comment
accordingly.
In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh`:
- Around line 263-294: The else branch assigning shared_quantiles is dead due to
the static_assert(kIsClassifier || kIsRegressor) in computeSplitKernel; remove
the unreachable else block (the branch that does shared_quantiles =
alignPointer<DataT>(shared_histogram + shared_histogram_len)) and keep the
existing assignments inside the kIsClassifier and kIsRegressor if constexpr
branches (or, alternatively, move a single shared_quantiles assignment after
those two constexpr branches if you prefer one place of initialization).
🪄 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: 55041c41-f738-4e8a-88f2-6d12ccb093bb
📒 Files selected for processing (32)
cpp/CMakeLists.txtcpp/bench/CMakeLists.txtcpp/bench/sg/rf_classifier.cucpp/bench/sg/rf_regressor.cucpp/include/cuml/ensemble/randomforest.hppcpp/src/decisiontree/batched-levelalgo/bins.cuhcpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/dataset.hcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhcpp/src/decisiontree/batched-levelalgo/objectives.cuhcpp/src/decisiontree/decisiontree.cuhcpp/src/randomforest/per_tree_weights.cucpp/src/randomforest/per_tree_weights.cuhcpp/src/randomforest/randomforest.cucpp/src/randomforest/randomforest.cuhcpp/tests/sg/rf_test.cudocs/source/cuml-accel/limitations.rstpython/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/cuml_accel_tests/test_onnx.pypython/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yamlpython/cuml/tests/dask/test_dask_random_forest.pypython/cuml/tests/test_random_forest.pypython/cuml/tests/test_sklearn_compatibility.pypython/cuml/tests/test_sklearn_import_export.py
💤 Files with no reviewable changes (1)
- python/cuml/cuml_accel_tests/test_onnx.py
switch527
force-pushed
the
enh-rf-classifier-class-weight
branch
2 times, most recently
from
June 2, 2026 03:51
4350fee to
9d0c03d
Compare
Towards NVIDIA#8093. Extends sample_weight to RandomForestRegressor.fit, building on the classifier work in NVIDIA#8132. Single-GPU only; distributed (Dask) raises NotImplementedError and points users at the single-GPU class (tracking NVIDIA#8186). Regressor mirrors the classifier companion-buffer pattern with a double* weighted_count_histograms alongside the existing AggregateBin histogram. The companion holds sum(weight) per bin and feeds Gain for the weighted denominator and SetLeafVector for the leaf mean. AggregateBin.count stays unweighted so min_samples_leaf and Split::nLeft still operate on integer sample counts. The four regressor objectives substitute n with sum(weight) following sklearn 1.7.2 _criterion.pyx proxy_impurity_improvement (MSE :1089-1118; Poisson :1597-1642); Gamma and InverseGaussian apply the same substitution to cuml's existing formulas. All six SetLeafVector paths NaN-guard the all-zero-weight leaf. Tests add per-objective weighted ground-truth gtests with hand-derived expected values, NaN-guard gtests for both SetLeafVector paths, and Python tests mirrored across RFC and RFR. Accel proxy forwards sample_weight to GPU. test_sklearn_compatibility.py and xfail-list.yaml xfail check_sample_weight_equivalence for both estimators (quantile binning != row duplication). Perf: RFR<double> MSE +11.1% on the rf_regressor bench vs the pre-PR baseline (extra per-sample atomicAdd into the companion buffer + extra pdf_to_cdf<double> scan); RFR<float> flat; classifier within noise. libcuml.so +36 KB. Adjacent fix: re-enabled cpp/bench/sg/rf_regressor.cu (disabled in a 2024 FIXME); pre-existing compile errors fixed and workload scaled down so it finishes in minutes.
Closes NVIDIA#8093, Refs NVIDIA#8146 Adds `class_weight` to `RandomForestClassifier`. Supports `None`, a dict, `'balanced'`, and `'balanced_subsample'`. The first three reuse the existing helper from LR/SVC. `'balanced_subsample'` re-weights per tree from each bootstrap sample (matches sklearn). With `bootstrap=False` it silently collapses to `'balanced'`, also matching sklearn. cuml exposes `class_weight_` as a fitted attribute (mirroring SVC); sklearn's RFC doesn't. Under weighted training, `feature_importances_` is well-formed but not byte-equivalent to sklearn's; cuml weights the impurity term, sklearn weights node counts. Documented in the `Notes` section. Overhead on a 500k x 30 x 10-class fixture: `balanced` is essentially free, `balanced_subsample` is +2.5%. The unweighted path stays byte-identical. Adjacent fix: `max_samples` now round-trips to `None` when `bootstrap=False` so the new round-trip tests on `balanced_subsample` succeed (sklearn rejects the combination). Tests cover each `class_weight` shape, refit, `predict_proba`, sklearn round-trip parametrized over `class_weight x bootstrap x oob_score`, plus stress coverage. Two upstream tests stay xfailed (importance-formula divergence; reliance on a cuml-unsupported parameter); both rationales name a revisit trigger. PR-4 (NVIDIA#8143) closes the ETC half of NVIDIA#8146 by inheritance.
switch527
force-pushed
the
enh-rf-classifier-class-weight
branch
from
June 2, 2026 15:37
9d0c03d to
c45e96e
Compare
Contributor
Author
|
@divyegala, it requires the previous PR in this series to function, but I think your team wanted to think through the implementation more so I'm going to go ahead and close it out |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #8093, Refs #8146
Adds
class_weighttoRandomForestClassifier. SupportsNone, a dict,'balanced', and'balanced_subsample'. The first three reuse the existing helper from LR/SVC.'balanced_subsample're-weights per tree from each bootstrap sample (matches sklearn). Withbootstrap=Falseit silently collapses to'balanced', also matching sklearn.cuml exposes
class_weight_as a fitted attribute (mirroring SVC); sklearn's RFC doesn't. Under weighted training,feature_importances_is well-formed but not byte-equivalent to sklearn's; cuml weights the impurity term, sklearn weights node counts. Documented in the docstring.Overhead on a 500k × 30 × 10-class fixture:
balancedis essentially free,balanced_subsampleis +2.5%. The unweighted path stays byte-identical.Adjacent fix:
max_samplesnow round-trips toNonewhenbootstrap=Falseso the new round-trip tests onbalanced_subsamplesucceed (sklearn rejects the combination).Tests cover each
class_weightshape, refit,predict_proba, sklearn round-trip parametrized overclass_weight × bootstrap × oob_score, plus stress coverage. Two upstream tests stay xfailed (importance-formula divergence; reliance on a cuml-unsupported parameter); both rationales name a revisit trigger.PR-4 (#8143) closes the ETC half of #8146 by inheritance.