RF: narrow CountBin to double for weighted-training prep - #8132
Merged
rapids-bot[bot] merged 4 commits intoJun 1, 2026
Conversation
switch527
force-pushed
the
fea-rf-sample-class-weight
branch
from
May 19, 2026 00:54
5f0b07a to
d8f7d0f
Compare
csadorf
added
improvement
switch527
marked this pull request as ready for review
May 19, 2026 21:04
|
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:
📝 WalkthroughWalkthroughAdds sample_weight/class_weight end-to-end: weighted bin types and objectives, deterministic pool-based cross-block reduction, per-node weighted counts, DecisionTree/RandomForest builder & kernel integration, Cython/Python API plumbing, Dask MNMG rejection for weights, and comprehensive C++/Python tests. ChangesWeighted Random Forest Training
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ 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: 3
🧹 Nitpick comments (2)
cpp/include/cuml/ensemble/randomforest.hpp (1)
153-179: ⚡ Quick winExpose
sample_weightasconstin the public API.Nothing in the training path mutates weights, but these new declarations require a mutable device buffer. That forces callers with const device memory to cast away constness and suggests an ownership/write contract the implementation does not have. Please make the new parameter
consthere and in the corresponding definitions/bindings.Also applies to: 237-261
🤖 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 153 - 179, Change the sample_weight parameters to be const since training does not mutate them: update the public declarations and corresponding definitions/bindings for the overloads of fit (the float* sample_weight and double* sample_weight parameters) to const float* and const double* respectively, and update the template fit_treelite signature's T* sample_weight to const T* (and any other occurrences mentioned around lines 237-261). Make corresponding changes in the matching implementation bodies and any C/API bindings so parameter types match the new const-qualified pointer types.cpp/tests/sg/rf_test.cu (1)
2147-2173: 🏗️ Heavy liftAdd a non-unit-weight oracle for
WeightedPoisson,WeightedGamma, andWeightedInverseGaussian.Those three new objective paths are only checked via the unit-weight anchor right now. An implementation that ignores non-unit weights, or miscomputes the weighted-count terms, would still pass this suite. Please add independent non-unit-weight reference gains for them as well, like the Gini/Entropy/MSE branches.
🤖 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/tests/sg/rf_test.cu` around lines 2147 - 2173, Add branches to the non-unit-weight hypothesis check for the three weighted exponential-family objectives (WeightedPoisson, WeightedGamma, WeightedInverseGaussian) analogous to the existing Gini/Entropy/MSE branches: inside the if constexpr chain (after WeightedMSEObjectiveFunction) add constexpr checks for the WeightedPoisson, WeightedGamma, and WeightedInverseGaussian objective types, compute the reference gain via the corresponding ground-truth functions (e.g. WeightedPoissonGroundTruthGain, WeightedGammaGroundTruthGain, WeightedInverseGaussianGroundTruthGain) using the same inputs (data, weights, split_bin), and assert equality with ASSERT_NEAR(gt, double(hyp_w), this->params.tolerance) guarded by the same NaN and sentinel checks (!std::isnan(gt), !std::isnan(double(hyp_w)), gt != -std::numeric_limits<double>::max()) so non-unit-weight behavior is independently verified.
🤖 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/include/cuml/tree/decisiontree.hpp`:
- Around line 98-101: The new public member weighted_node_count added to
TreeMetaDataNode changes the ABI/aggregate layout and breaks downstream C++
callers; remove weighted_node_count from the public TreeMetaDataNode definition
and instead store it either in a separate internal-only struct or behind an
accessor/versioned wrapper (e.g., keep TreeMetaDataNode unchanged, create an
internal TreeMetaDataNodeImpl or TreeMetaDataNodeV2 that holds
weighted_node_count, or add private storage accessed via a getter like
getWeightedNodeCount() exposed only in internal headers) so the public aggregate
shape remains stable while still providing the weighted_node_count metadata to
internal code paths.
In `@cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh`:
- Around line 171-174: Add immediate CUDA launch error checks after kernel
launches in launchWeightedLeftCountKernel and launchComputeSplitKernel: after
the invocation of weightedLeftCountKernel<<<...>>>(...) in
launchWeightedLeftCountKernel and after the compute split kernel launch in
launchComputeSplitKernel, call RAFT_CUDA_TRY(cudaPeekAtLastError()); to surface
launch/configuration errors early; locate the kernel launch sites by the symbols
weightedLeftCountKernel and the compute split kernel invocation inside those
functions and insert the RAFT_CUDA_TRY(cudaPeekAtLastError()); call immediately
following each <<<...>>> launch.
In `@cpp/src/randomforest/randomforest.cuh`:
- Around line 121-122: The new sample_weight parameter must be validated as a
device pointer before any device-side consumption; update the
caller/implementation that declares bool* bootstrap_masks and const T*
sample_weight (the function taking these params) to check sample_weight when
non-null using CUDA pointer attributes (e.g., via cudaPointerGetAttributes or
your existing is_device_pointer helper) and assert/log+return an error if it is
a host pointer or attributes query fails so thrust::transform_reduce and
subsequent tree kernels never receive a host pointer.
---
Nitpick comments:
In `@cpp/include/cuml/ensemble/randomforest.hpp`:
- Around line 153-179: Change the sample_weight parameters to be const since
training does not mutate them: update the public declarations and corresponding
definitions/bindings for the overloads of fit (the float* sample_weight and
double* sample_weight parameters) to const float* and const double*
respectively, and update the template fit_treelite signature's T* sample_weight
to const T* (and any other occurrences mentioned around lines 237-261). Make
corresponding changes in the matching implementation bodies and any C/API
bindings so parameter types match the new const-qualified pointer types.
In `@cpp/tests/sg/rf_test.cu`:
- Around line 2147-2173: Add branches to the non-unit-weight hypothesis check
for the three weighted exponential-family objectives (WeightedPoisson,
WeightedGamma, WeightedInverseGaussian) analogous to the existing
Gini/Entropy/MSE branches: inside the if constexpr chain (after
WeightedMSEObjectiveFunction) add constexpr checks for the WeightedPoisson,
WeightedGamma, and WeightedInverseGaussian objective types, compute the
reference gain via the corresponding ground-truth functions (e.g.
WeightedPoissonGroundTruthGain, WeightedGammaGroundTruthGain,
WeightedInverseGaussianGroundTruthGain) using the same inputs (data, weights,
split_bin), and assert equality with ASSERT_NEAR(gt, double(hyp_w),
this->params.tolerance) guarded by the same NaN and sentinel checks
(!std::isnan(gt), !std::isnan(double(hyp_w)), gt !=
-std::numeric_limits<double>::max()) so non-unit-weight behavior is
independently verified.
🪄 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: 73b0e62f-2773-4550-81ca-e58ecb6a5232
📒 Files selected for processing (38)
cpp/CMakeLists.txtcpp/include/cuml/ensemble/randomforest.hppcpp/include/cuml/tree/decisiontree.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/kernels/weighted_entropy-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_entropy-float.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_gamma-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_gamma-float.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_gini-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_gini-float.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_inverse_gaussian-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_inverse_gaussian-float.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_mse-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_mse-float.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_poisson-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted_poisson-float.cucpp/src/decisiontree/batched-levelalgo/objectives.cuhcpp/src/decisiontree/decisiontree.cuhcpp/src/randomforest/randomforest.cucpp/src/randomforest/randomforest.cuhcpp/tests/sg/rf_test.cupython/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/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
Contributor
Author
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cpp/src/randomforest/randomforest.cuh (2)
121-122: ⚡ Quick winDocument
sample_weightin the public API comment.
fit()now exposessample_weight, but the Doxygen block still ends atbootstrap_masks. Please add an@param[in] sample_weightentry so the generated docs match the signature.📝 Suggested doc update
* `@param`[out] bootstrap_masks: optional device pointer to store bootstrap masks * (n_trees * n_rows), only populated if a non-null pointer is provided + * `@param`[in] sample_weight: optional device pointer with per-row training + * weights (n_rows elements). When null, all rows are weighted equally. */🤖 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/randomforest/randomforest.cuh` around lines 121 - 122, The public Doxygen for the RandomForest fit() overload is missing documentation for the new parameter sample_weight; update the comment block that currently documents bootstrap_masks to add an `@param`[in] sample_weight entry describing that it is a pointer to per-sample weights (or nullptr for uniform weights) so the generated API docs match the fit(const T* sample_weight, bool* bootstrap_masks, ...) signature; ensure you reference the same names used in the function declaration (fit, sample_weight, bootstrap_masks) and keep wording consistent with existing param descriptions.
172-228: ⚡ Quick winStop scheduling more trees after the first captured exception.
Once
build_excis set, later iterations still enterDecisionTree::fiteven thoughfit()will rethrow at the end. That does extra work on a failed call and can leave more partial state inforest->trees/bootstrap_masksthan necessary.🛑 One way to short-circuit later iterations
+#include <atomic> `#include` <exception> `#include` <map> @@ - std::exception_ptr build_exc; + std::exception_ptr build_exc; + std::atomic<bool> cancel_build{false}; @@ `#pragma` omp parallel for num_threads(n_streams) for (int i = 0; i < this->rf_params.n_trees; i++) { + if (cancel_build.load(std::memory_order_acquire)) { continue; } try { int stream_id = omp_get_thread_num(); auto s = handle.get_stream_from_stream_pool(stream_id); @@ } catch (...) { `#pragma` omp critical { - if (!build_exc) build_exc = std::current_exception(); + if (!build_exc) { + build_exc = std::current_exception(); + cancel_build.store(true, std::memory_order_release); + } } } }🤖 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/randomforest/randomforest.cuh` around lines 172 - 228, Add a short-circuit flag to stop scheduling work once an exception is captured: introduce a shared std::atomic<bool> build_failed{false}; at the top of the omp parallel for loop check if build_failed.load() and immediately continue (before calling DecisionTree::fit, get_stream_from_stream_pool, or touching forest->trees/bootstrap_masks); in the catch(...) set build_failed.store(true) (inside the existing omp critical where build_exc is set) so remaining iterations skip expensive work and avoid partial writes to forest->trees or bootstrap_masks. Ensure the checks reference build_failed, DecisionTree::fit, forest->trees, bootstrap_masks, and build_exc so the short-circuit happens before any call to fit().
🤖 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.
Nitpick comments:
In `@cpp/src/randomforest/randomforest.cuh`:
- Around line 121-122: The public Doxygen for the RandomForest fit() overload is
missing documentation for the new parameter sample_weight; update the comment
block that currently documents bootstrap_masks to add an `@param`[in]
sample_weight entry describing that it is a pointer to per-sample weights (or
nullptr for uniform weights) so the generated API docs match the fit(const T*
sample_weight, bool* bootstrap_masks, ...) signature; ensure you reference the
same names used in the function declaration (fit, sample_weight,
bootstrap_masks) and keep wording consistent with existing param descriptions.
- Around line 172-228: Add a short-circuit flag to stop scheduling work once an
exception is captured: introduce a shared std::atomic<bool> build_failed{false};
at the top of the omp parallel for loop check if build_failed.load() and
immediately continue (before calling DecisionTree::fit,
get_stream_from_stream_pool, or touching forest->trees/bootstrap_masks); in the
catch(...) set build_failed.store(true) (inside the existing omp critical where
build_exc is set) so remaining iterations skip expensive work and avoid partial
writes to forest->trees or bootstrap_masks. Ensure the checks reference
build_failed, DecisionTree::fit, forest->trees, bootstrap_masks, and build_exc
so the short-circuit happens before any call to fit().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0ca210e1-7925-440e-a3d2-2ddb7db120f0
📒 Files selected for processing (1)
cpp/src/randomforest/randomforest.cuh
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 22, 2026
Closes NVIDIA#8133. Adds `ExtraTreesClassifier` and `ExtraTreesRegressor` to `cuml.ensemble`. Both subclass the existing RandomForest estimators, route through a new `SPLITTER_RANDOM` C++ path, and default to `bootstrap=False` to match scikit-learn's ExtraTrees defaults. `sample_weight` and `class_weight` reuse the parent estimators' machinery. The C++ side is a direct-single-threshold split kernel that skips the per-bin histogram. Splitter-aware dispatch in `Builder::computeSplit` keeps the `SPLITTER_BEST` arm byte-identical; the `SPLITTER_RANDOM` arm uses a smaller per-block scratch and reuses the deterministic pool primitive introduced by NVIDIA#8132. Adjacent fix: `BaseRandomForestModel._params_to_cpu` coerces `max_samples` to `None` when `bootstrap=False` so the sklearn round-trip succeeds. Affects RandomForest too; sklearn rejects that combination anyway. Test coverage adds a new C++ gtest binary plus Python sklearn-parity, round-trip, and accel-integration tests for both estimators. Stacks on NVIDIA#8132 since the weighted bin types and pool primitive live there. The Python entry point routes through the existing RandomForest code path unchanged; `_splitter` is a class attribute, not a user-facing parameter.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 22, 2026
Closes NVIDIA#8133. Adds `ExtraTreesClassifier` and `ExtraTreesRegressor` to `cuml.ensemble`. Both subclass the existing RandomForest estimators, route through a new `SPLITTER_RANDOM` C++ path, and default to `bootstrap=False` to match scikit-learn's ExtraTrees defaults. `sample_weight` and `class_weight` reuse the parent estimators' machinery. The C++ side is a direct-single-threshold split kernel that skips the per-bin histogram. Splitter-aware dispatch in `Builder::computeSplit` keeps the `SPLITTER_BEST` arm byte-identical; the `SPLITTER_RANDOM` arm uses a smaller per-block scratch and reuses the deterministic pool primitive introduced by NVIDIA#8132. Adjacent fix: `BaseRandomForestModel._params_to_cpu` coerces `max_samples` to `None` when `bootstrap=False` so the sklearn round-trip succeeds. Affects RandomForest too; sklearn rejects that combination anyway. Test coverage adds a new C++ gtest binary plus Python sklearn-parity, round-trip, and accel-integration tests for both estimators. Stacks on NVIDIA#8132 since the weighted bin types and pool primitive live there. The Python entry point routes through the existing RandomForest code path unchanged; `_splitter` is a class attribute, not a user-facing parameter.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 22, 2026
Closes NVIDIA#8133. Adds `ExtraTreesClassifier` and `ExtraTreesRegressor` to `cuml.ensemble`. Both subclass the existing RandomForest estimators, route through a new `SPLITTER_RANDOM` C++ path, and default to `bootstrap=False` to match scikit-learn's ExtraTrees defaults. `sample_weight` and `class_weight` reuse the parent estimators' machinery. The C++ side is a direct-single-threshold split kernel that skips the per-bin histogram. Splitter-aware dispatch in `Builder::computeSplit` keeps the `SPLITTER_BEST` arm byte-identical; the `SPLITTER_RANDOM` arm uses a smaller per-block scratch and reuses the deterministic pool primitive introduced by NVIDIA#8132. Adjacent fix: `BaseRandomForestModel._params_to_cpu` coerces `max_samples` to `None` when `bootstrap=False` so the sklearn round-trip succeeds. Affects RandomForest too; sklearn rejects that combination anyway. Test coverage adds a new C++ gtest binary plus Python sklearn-parity, round-trip, and accel-integration tests for both estimators. Stacks on NVIDIA#8132 since the weighted bin types and pool primitive live there. The Python entry point routes through the existing RandomForest code path unchanged; `_splitter` is a class attribute, not a user-facing parameter.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 22, 2026
Closes NVIDIA#8133. Adds `ExtraTreesClassifier` and `ExtraTreesRegressor` to `cuml.ensemble`. Both subclass the existing RandomForest estimators, route through a new `SPLITTER_RANDOM` C++ path, and default to `bootstrap=False` to match scikit-learn's ExtraTrees defaults. `sample_weight` and `class_weight` reuse the parent estimators' machinery. The C++ side is a direct-single-threshold split kernel that skips the per-bin histogram. Splitter-aware dispatch in `Builder::computeSplit` keeps the `SPLITTER_BEST` arm byte-identical; the `SPLITTER_RANDOM` arm uses a smaller per-block scratch and reuses the deterministic pool primitive introduced by NVIDIA#8132. Adjacent fix: `BaseRandomForestModel._params_to_cpu` coerces `max_samples` to `None` when `bootstrap=False` so the sklearn round-trip succeeds. Affects RandomForest too; sklearn rejects that combination anyway. Test coverage adds a new C++ gtest binary plus Python sklearn-parity, round-trip, and accel-integration tests for both estimators. Stacks on NVIDIA#8132 since the weighted bin types and pool primitive live there. The Python entry point routes through the existing RandomForest code path unchanged; `_splitter` is a class attribute, not a user-facing parameter.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 22, 2026
Closes NVIDIA#8133. Adds `ExtraTreesClassifier` and `ExtraTreesRegressor` to `cuml.ensemble`. Both subclass the existing RandomForest estimators, route through a new `SPLITTER_RANDOM` C++ path, and default to `bootstrap=False` to match scikit-learn's ExtraTrees defaults. `sample_weight` and `class_weight` reuse the parent estimators' machinery. The C++ side is a direct-single-threshold split kernel that skips the per-bin histogram. Splitter-aware dispatch in `Builder::computeSplit` keeps the `SPLITTER_BEST` arm byte-identical; the `SPLITTER_RANDOM` arm uses a smaller per-block scratch and reuses the deterministic pool primitive introduced by NVIDIA#8132. Adjacent fix: `BaseRandomForestModel._params_to_cpu` coerces `max_samples` to `None` when `bootstrap=False` so the sklearn round-trip succeeds. Affects RandomForest too; sklearn rejects that combination anyway. Test coverage adds a new C++ gtest binary plus Python sklearn-parity, round-trip, and accel-integration tests for both estimators. Stacks on NVIDIA#8132 since the weighted bin types and pool primitive live there. The Python entry point routes through the existing RandomForest code path unchanged; `_splitter` is a class attribute, not a user-facing parameter.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 25, 2026
Closes NVIDIA#8133. Adds `ExtraTreesClassifier` and `ExtraTreesRegressor` to `cuml.ensemble`. Both subclass the existing RandomForest estimators, route through a new `SPLITTER_RANDOM` C++ path, and default to `bootstrap=False` to match scikit-learn's ExtraTrees defaults. `sample_weight` and `class_weight` reuse the parent estimators' machinery. The C++ side is a direct-single-threshold split kernel that skips the per-bin histogram. Splitter-aware dispatch in `Builder::computeSplit` keeps the `SPLITTER_BEST` arm byte-identical; the `SPLITTER_RANDOM` arm uses a smaller per-block scratch and reuses the deterministic pool primitive introduced by NVIDIA#8132. Adjacent fix: `BaseRandomForestModel._params_to_cpu` coerces `max_samples` to `None` when `bootstrap=False` so the sklearn round-trip succeeds. Affects RandomForest too; sklearn rejects that combination anyway. Test coverage adds a new C++ gtest binary plus Python sklearn-parity, round-trip, and accel-integration tests for both estimators. Stacks on NVIDIA#8132 since the weighted bin types and pool primitive live there. The Python entry point routes through the existing RandomForest code path unchanged; `_splitter` is a class attribute, not a user-facing parameter.
switch527
force-pushed
the
fea-rf-sample-class-weight
branch
from
May 25, 2026 18:04
3a32039 to
9536290
Compare
switch527
force-pushed
the
fea-rf-sample-class-weight
branch
from
May 30, 2026 22:56
6011012 to
e1db877
Compare
switch527
marked this pull request as ready for review
May 30, 2026 23:02
RAMitchell
approved these changes
May 31, 2026
| p.data.ncols = rc.ncols; | ||
| p.data.nclasses = rc.nclasses; | ||
| p.rf.tree_params.max_features = 1.f / std::sqrt(float(rc.ncols)); | ||
| if (!std::is_same<D, float>::value && cfg.ncols == 968) continue; |
Contributor
There was a problem hiding this comment.
Is this condition still needed?
Contributor
Author
There was a problem hiding this comment.
Bosch double is ~9 GB for X alone, the skip is there so the bench runs on smaller cards. Will drop if you'd rather always run both dtypes.
switch527
changed the title
CountBin to float for weighted-training prepCountBin to double for weighted-training prep
switch527
added a commit
to switch527/cuml
that referenced
this pull request
May 31, 2026
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
csadorf
approved these changes
Jun 1, 2026
csadorf
left a comment
csadorf
left a comment
Contributor
There was a problem hiding this comment.
Approving this, but I still have some reservations about the performance tradeoff.
I do not agree that it is obvious that we should use int64 when the narrower count type is sufficient, and a 20-70% regression is significant enough that we should not treat it as irrelevant. I am okay with this PR moving forward to unblock the weighted-training work, but I think we should re-assess the performance impact after that lands and consider adding an int-based fast path for the unweighted case if the regression holds up.
I had to push a small cleanup commit for linter issues. Please make sure to install and run pre-commit locally.
I also reverted the removed RandomForestClassifier ONNX xfail. I suspect you were running with an older unaffected ONNX-version. Our xfail should probably be version-dependent. We can fix that up independently, i.e., not in this PR.
Contributor
|
/merge |
Contributor
Author
|
Totally understand the concern. I can throw in an int fast path in after I finish up the ExtraTrees stuff if it's a desired feature |
switch527
added a commit
to switch527/cuml
that referenced
this pull request
Jun 2, 2026
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.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
Jun 2, 2026
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.
switch527
added a commit
to switch527/cuml
that referenced
this pull request
Jun 2, 2026
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.
rapids-bot Bot
pushed a commit
that referenced
this pull request
Jun 11, 2026
## Summary Refs [#8093](#8093), [#1279](#1279). Builds on [#8132](#8132) and [#8233](#8233). Related follow-ups: [#8186](#8186), [#8146](#8146). This PR prepares the RF objective/bin layer for weighted training without threading `sample_weight` through the public estimator APIs yet. - Renames RF histogram bins to `ClassificationBin` and `RegressionBin` - Adds weighted bin variants that preserve integer sample counts while separately accumulating sample weight - Switches objective families to a `weighted` bool template parameter - Expands objective tests across weighted and unweighted regression/classification criteria - Adds weighted ground-truth checks for MSE, Poisson, Gamma, Inverse Gaussian, Entropy, and Gini ## Notes This is groundwork only. It does not yet route `sample_weight` through Python/Cython, sampling, or RF training kernels. ## Testing - `git diff --check` - `SG_RF_TEST.localnvforest --gtest_filter="*ObjectiveTest*"` - 96 tests passed A full normal `ninja -C cpp/build-ninja-gcc12 SG_RF_TEST -j2` run was started after reconfiguring the local build cache to use the local nvforest artifact, but was paused before completion. Authors: - Rory Mitchell (https://github.com/RAMitchell) Approvers: - Philip Hyunsu Cho (https://github.com/hcho3) URL: #8247
This was referenced Jul 6, 2026
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
5 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.
Refs #8093. First of three PRs in the RF weighted-training series, force-pushed to replace the prior Design D float-bin commit per RAMitchell's review direction to use double rather than float (the original int32 had overflow concerns).
Widens
CountBin::xfrominttodoubleso the remaining PRs (sample_weight, balanced_subsample, ExtraTrees) can share one unified bin type without a parallel weighted code path. Accumulators inGainPerSplitandSetLeafVectorpromoted todoubleto match. No CUB fast-path tricks; the unifiedBlockScan<BinT>path handles bothCountBinandAggregateBin.Bench data on Blackwell sm_120 across 18 RFClassifier configs (5-rep median):
For reference, the int64 comparison RAMitchell suggested in the review thread: min +2.7%, median +8.4%, max +50.3%. The gap between int64 and double (~12% median) is the residual cost of double-precision math vs 64-bit integer math on top of the shared 8-byte storage cost. Worst case is the high-class-count config (n_bins=128, n_classes=44) at +70.5%; bandwidth doubles at high class count, which is the structural cost.
Several optimization candidates were explored (CUB algorithm variants, hand-rolled warp-shuffle scan, per-warp histograms, fused scan+gain). The top-ranked candidate (a
reinterpret_cast<double*>to route CUB through its primitive-scalar code path) was prototyped and showed no measurable gain. The rest are documented in case a real perf concern surfaces later.Includes two adjacent cleanups: a
static_cast<IdxT>in theObjectiveTestfixture inrf_test.cufor the int-to-IdxT narrowing the bin-type change surfaces, and removal of a stalexfail_skl2onnx_bugon RandomForestClassifier intest_onnx.py. The xfail was XPASSing under strict mode after the bin-type change unblocked the skl2onnx round-trip path for the classifier; the un-xfail keeps the test green going forward.Verification: 180/180
SG_RF_TESTgtests pass on the minimal-double binary (Blackwell sm_120), including thePropertyBasedTestdeterminism checks across 100 parameterized configs.