RF: add sample_weight to RandomForestRegressor - #8187
Closed
switch527 wants to merge 1 commit into
Closed
Conversation
|
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:
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 (26)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (21)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThreads optional per-sample weights across decision-tree histograms, kernels, objectives, RandomForest/DecisionTree C++ APIs, Cython/Python bindings, and tests; Dask estimators explicitly reject non-None sample_weight; docs and benchmarks updated. ChangesPer-Sample Weighting Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cpp/src/randomforest/randomforest.cuh (1)
123-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
sample_weightpointer before training starts.Line 124 validates
input/labels, but the newsample_weightpointer is forwarded unchecked. A host pointer here can trigger invalid device-memory access in downstream training code.Suggested patch
raft::common::nvtx::range fun_scope("RandomForest::fit `@randomforest.cuh`"); this->error_checking(input, labels, n_rows, n_cols, false); + if (sample_weight != nullptr) { + ASSERT(DT::is_dev_ptr(sample_weight), + "RF Error: Expected sample_weight to be a GPU pointer"); + } const raft::handle_t& handle = user_handle;🤖 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 123 - 125, The fit entrypoint (RandomForest::fit) currently calls this->error_checking(input, labels, n_rows, n_cols, false) but does not validate the new sample_weight pointer; add a validation step before proceeding (similar to input/labels checks) to ensure sample_weight is either nullptr or a valid host/device pointer as expected by downstream code and raise/return an error if it's invalid; modify the RandomForest::fit pre-checks to include sample_weight validation (or extend error_checking to accept sample_weight) so downstream training routines never receive an unchecked host pointer that could cause invalid device-memory access.cpp/include/cuml/ensemble/randomforest.hpp (2)
145-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the old exported signatures as deprecation shims.
Appending
sample_weighthere changes the public C++ training signatures, which changes the mangled names for these exported entry points. That is an API/ABI break for existinglibcumlconsumers even though recompiled call sites can omit the new argument. Please keep the previous overloads as deprecated forwarders (or explicitly version/migrate this change) instead of replacing the signatures in place. As per coding guidelines, public headers undercpp/include/cuml/**/*must avoid breaking API changes without proper deprecation warnings.Also applies to: 169-180, 230-249, 251-262
🤖 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 145 - 166, The new optional sample_weight parameters changed the exported mangled names for the public fit overloads (e.g., fit(const raft::handle_t&, RandomForestClassifierF*, float*, int, int, int*, int, RF_params, rapids_logger::level_enum, bool*, float*) and the double/ClassifierD variants); restore the previous public signatures by adding back the original overloads without sample_weight as deprecated forwarders that call the new implementations (or provide versioned wrappers), mark them deprecated, and keep the new overloads with sample_weight as the canonical implementation; apply the same treatment to the other changed overload groups referenced in the comment (lines for the other float/double and prediction/multi-class fit variants) so the ABI is preserved while guiding users to the new APIs.
145-166: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDocument the
sample_weightcontract in this header.These public declarations now expose a new raw pointer without any parameter docs. Please add at least minimal Doxygen covering expected length, device/host memory expectations,
nullptrbehavior, and whether zero or negative weights are valid so downstream C++ callers have the same contract the Python layer already relies on. As per coding guidelines, public headers undercpp/include/cuml/**/*require Doxygen documentation for public functions and API changes to be flagged for docs updates.Also applies to: 169-180, 230-249, 251-262
🤖 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 145 - 166, Add Doxygen for the sample_weight parameter on the public fit overloads (RandomForestClassifierF::fit, RandomForestClassifierD::fit and the other affected overloads at the noted ranges) describing that sample_weight is an optional pointer of length n_rows, whether it must point to device or host memory (specify device memory if GPU-only, or host if copied), that nullptr disables weighting, and the semantics for zero/negative values (e.g., zero means exclude the sample, negative values invalid or treated as error) plus expected numeric type (float/double) and ownership (caller retains ownership). Ensure the docblock is added to each function declaration so the header documents the exact contract used by the Python layer and downstream C++ callers.
🤖 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_accel_tests/integration/test_rf_classifier.py`:
- Around line 222-224: Replace the nondeterministic assertion by computing
predictions and comparing the weighted score to sklearn.metrics.accuracy_score:
call y_pred = clf.predict(X), compute expected = accuracy_score(y, y_pred,
sample_weight=w), and assert s_weighted == expected (or use pytest.approx if
floating-point tolerance is needed); reference the variables clf.score,
s_weighted, and use accuracy_score to perform the deterministic check.
In `@python/cuml/cuml_accel_tests/integration/test_rf_regressor.py`:
- Around line 206-208: Replace the flaky inequality check between reg.score(...)
variants with a deterministic metric equivalence: compute the weighted R² via
r2_score(y, reg.predict(X), sample_weight=w) and compare that expected value to
reg.score(X, y, sample_weight=w) using a tolerant numeric assertion (e.g.,
np.testing.assert_allclose or pytest.approx) so the test verifies equality to
the metric implementation rather than relying on !=; reference the reg.score
method, r2_score, and the sample_weight w when updating the assertion.
In `@python/cuml/cuml/dask/ensemble/randomforestregressor.py`:
- Around line 144-152: The fit method currently allows sample_weight as a
positional argument which shifts subsequent positional arguments (convert_dtype)
for callers; change the signature of fit so sample_weight is keyword-only by
moving it after the existing * (i.e., make parameters sample_weight,
convert_dtype, broadcast_data all keyword-only), update any internal references
if needed, and run tests to ensure existing callers that passed sample_weight
positionally now pass it by keyword; target the fit method and the parameters
sample_weight and convert_dtype when making this change.
In `@python/cuml/tests/dask/test_dask_random_forest.py`:
- Around line 523-527: The helper _tiny_dataset currently creates two partitions
which can produce empty partitions on multi-worker setups; change its Dask array
creation to use a single chunk equal to the number of samples so partitions are
topology-agnostic: for X and y call from_array(..., chunks=len(y)) (or
chunks=(len(y),) for 2D) and also wrap the sample-weight w with from_array(w,
chunks=len(y)) so all returned arrays have a single non-empty partition; apply
the same change to the other helper/test helpers referenced in the 530-565
range.
In `@python/cuml/tests/test_random_forest.py`:
- Around line 1538-1541: Replace the brittle inequality check by computing the
expected metrics with sklearn.metrics.accuracy_score and comparing clf.score
outputs to those expected values: call accuracy_score(y, clf.predict(X)) for the
unweighted expected score and accuracy_score(y, clf.predict(X), sample_weight=w)
for the weighted expected score, then assert s_unweighted == expected_unweighted
and s_weighted == expected_weighted; update the assertions around clf.score,
s_unweighted, s_weighted, X, y, w to use these deterministic comparisons (apply
same change at the other occurrence around s_unweighted/s_weighted at lines
~1593-1595).
---
Outside diff comments:
In `@cpp/include/cuml/ensemble/randomforest.hpp`:
- Around line 145-166: The new optional sample_weight parameters changed the
exported mangled names for the public fit overloads (e.g., fit(const
raft::handle_t&, RandomForestClassifierF*, float*, int, int, int*, int,
RF_params, rapids_logger::level_enum, bool*, float*) and the double/ClassifierD
variants); restore the previous public signatures by adding back the original
overloads without sample_weight as deprecated forwarders that call the new
implementations (or provide versioned wrappers), mark them deprecated, and keep
the new overloads with sample_weight as the canonical implementation; apply the
same treatment to the other changed overload groups referenced in the comment
(lines for the other float/double and prediction/multi-class fit variants) so
the ABI is preserved while guiding users to the new APIs.
- Around line 145-166: Add Doxygen for the sample_weight parameter on the public
fit overloads (RandomForestClassifierF::fit, RandomForestClassifierD::fit and
the other affected overloads at the noted ranges) describing that sample_weight
is an optional pointer of length n_rows, whether it must point to device or host
memory (specify device memory if GPU-only, or host if copied), that nullptr
disables weighting, and the semantics for zero/negative values (e.g., zero means
exclude the sample, negative values invalid or treated as error) plus expected
numeric type (float/double) and ownership (caller retains ownership). Ensure the
docblock is added to each function declaration so the header documents the exact
contract used by the Python layer and downstream C++ callers.
In `@cpp/src/randomforest/randomforest.cuh`:
- Around line 123-125: The fit entrypoint (RandomForest::fit) currently calls
this->error_checking(input, labels, n_rows, n_cols, false) but does not validate
the new sample_weight pointer; add a validation step before proceeding (similar
to input/labels checks) to ensure sample_weight is either nullptr or a valid
host/device pointer as expected by downstream code and raise/return an error if
it's invalid; modify the RandomForest::fit pre-checks to include sample_weight
validation (or extend error_checking to accept sample_weight) so downstream
training routines never receive an unchecked host pointer that could cause
invalid device-memory access.
🪄 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: 8eb93e75-d4b8-43cf-b988-d5c1830f2bfd
📒 Files selected for processing (28)
cpp/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/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.py
💤 Files with no reviewable changes (2)
- python/cuml/cuml_accel_tests/test_onnx.py
- docs/source/cuml-accel/limitations.rst
switch527
force-pushed
the
enh-rf-sample-weight
branch
from
May 31, 2026 23:27
33a5cbe to
36264ad
Compare
Contributor
Author
|
Pushed 36264ad addressing the five CodeRabbit threads in one commit:
Affected unit, accel, and dask suites pass locally. Backup tag |
switch527
force-pushed
the
enh-rf-sample-weight
branch
from
June 2, 2026 03:50
36264ad to
a2d268b
Compare
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 `@cpp/tests/sg/rf_test.cu`:
- Around line 741-752: The test comparing regressor forests (variables
forest_null and forest_ones) is missing assertions for tree depth_counter and
sparsetree size; update the tree-comparison loop in rf_test.cu (where tn and to
are compared) to also ASSERT_EQ(tn->depth_counter, to->depth_counter) and
ASSERT_EQ(tn->sparsetree.size(), to->sparsetree.size()) with the same diagnostic
text style used for leaf_counter and vector_leaf so the regressor unit-weight
test checks identical tree structure byte-for-byte.
🪄 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: ababa898-dceb-4bcc-b431-849679c47ddb
📒 Files selected for processing (26)
cpp/bench/CMakeLists.txtcpp/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/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/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.py
💤 Files with no reviewable changes (1)
- docs/source/cuml-accel/limitations.rst
🚧 Files skipped from review as they are similar to previous changes (22)
- cpp/src/decisiontree/batched-levelalgo/dataset.h
- cpp/bench/CMakeLists.txt
- cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh
- python/cuml/cuml_accel_tests/integration/test_rf_regressor.py
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
- cpp/bench/sg/rf_regressor.cu
- python/cuml/cuml_accel_tests/integration/test_rf_classifier.py
- cpp/src/randomforest/randomforest.cuh
- cpp/include/cuml/ensemble/randomforest.hpp
- cpp/src/decisiontree/decisiontree.cuh
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml/ensemble/randomforestclassifier.py
- cpp/src/decisiontree/batched-levelalgo/bins.cuh
- python/cuml/cuml/ensemble/randomforestregressor.py
- cpp/src/randomforest/randomforest.cu
- python/cuml/tests/test_random_forest.py
- cpp/src/decisiontree/batched-levelalgo/builder.cuh
- python/cuml/tests/dask/test_dask_random_forest.py
- python/cuml/cuml/accel/_overrides/sklearn/ensemble.py
- python/cuml/cuml/ensemble/randomforest_common.pyx
- cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh
- cpp/src/decisiontree/batched-levelalgo/objectives.cuh
switch527
force-pushed
the
enh-rf-sample-weight
branch
from
June 2, 2026 04:35
a2d268b to
8fcce04
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/decisiontree/batched-levelalgo/objectives.cuh (1)
319-329: 💤 Low valueConsider extracting common regression
SetLeafVectorlogic.The
SetLeafVectorimplementations for MSE, Poisson, Gamma, and InverseGaussian are identical. If the regression objectives continue to share this pattern, a shared helper or base class method could reduce future maintenance burden.This is a minor observation—the current duplication is manageable and the code is clear.
Also applies to: 420-431, 518-528, 615-625
🤖 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/objectives.cuh` around lines 319 - 329, Multiple regression objective implementations duplicate the same SetLeafVector logic; extract a shared helper function (e.g., a templated inline helper SetLeafVectorCommon or move into a common base used by MSE/Possion/Gamma/InverseGaussian) that accepts BinT const* shist, int nclasses, DataT* out, double weighted_total and contains the current guard for weighted_total <= 0.0 and the loop assigning out[i] = DataT(shist[i].label_sum / weighted_total). Replace the per-objective SetLeafVector implementations (the functions named SetLeafVector in these objective files) to call that helper to remove duplication while preserving types and behavior.
🤖 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/decisiontree/batched-levelalgo/objectives.cuh`:
- Around line 319-329: Multiple regression objective implementations duplicate
the same SetLeafVector logic; extract a shared helper function (e.g., a
templated inline helper SetLeafVectorCommon or move into a common base used by
MSE/Possion/Gamma/InverseGaussian) that accepts BinT const* shist, int nclasses,
DataT* out, double weighted_total and contains the current guard for
weighted_total <= 0.0 and the loop assigning out[i] = DataT(shist[i].label_sum /
weighted_total). Replace the per-objective SetLeafVector implementations (the
functions named SetLeafVector in these objective files) to call that helper to
remove duplication while preserving types and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b9c91de9-3c58-4696-8ec8-a66751ecc74f
📒 Files selected for processing (26)
cpp/bench/CMakeLists.txtcpp/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/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/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.py
💤 Files with no reviewable changes (1)
- docs/source/cuml-accel/limitations.rst
🚧 Files skipped from review as they are similar to previous changes (22)
- cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels.cuh
- cpp/bench/CMakeLists.txt
- python/cuml/cuml_accel_tests/integration/test_rf_classifier.py
- cpp/src/randomforest/randomforest.cuh
- python/cuml/tests/dask/test_dask_random_forest.py
- python/cuml/tests/test_sklearn_compatibility.py
- python/cuml/cuml/dask/ensemble/randomforestregressor.py
- cpp/include/cuml/ensemble/randomforest.hpp
- python/cuml/cuml/ensemble/randomforest_common.pyx
- cpp/src/decisiontree/batched-levelalgo/builder.cuh
- python/cuml/cuml/ensemble/randomforestclassifier.py
- cpp/src/decisiontree/decisiontree.cuh
- python/cuml/cuml_accel_tests/upstream/scikit-learn/xfail-list.yaml
- python/cuml/tests/test_random_forest.py
- python/cuml/cuml/ensemble/randomforestregressor.py
- python/cuml/cuml/dask/ensemble/randomforestclassifier.py
- cpp/bench/sg/rf_regressor.cu
- python/cuml/cuml/accel/_overrides/sklearn/ensemble.py
- python/cuml/cuml_accel_tests/integration/test_rf_regressor.py
- cpp/src/randomforest/randomforest.cu
- cpp/tests/sg/rf_test.cu
- cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh
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
force-pushed
the
enh-rf-sample-weight
branch
from
June 2, 2026 15:23
8fcce04 to
894ef35
Compare
Contributor
|
/ok to test 894ef35 |
RAMitchell
requested changes
Jun 3, 2026
RAMitchell
left a comment
RAMitchell
left a comment
Contributor
There was a problem hiding this comment.
This PR is way too large again and is pretty hacky.
The dilemma here is that we might need both integer counts or sums of weights to implement both min_samples_leaf and min_weight_leaf.
We could:
- Create new templated bins/objectives with weighted versions for every objective. This would double compile time and binary size.
- We expand the bin types to all include both integer count and weight accumulators. This will increase memory and presumably decrease performance across the board.
- Create a separate buffer like you have done here, but this creates branches everywhere and is hard to read and maintain.
- We diverge slightly from sklearn and min_samples means mininum weight sum in the weighted case. Implementation is elegant and fast. This is pretty reasonable but just a slightly different behaviour.
Contributor
Author
|
Option 4 sounds right. Will drop the companion buffers here, on #8188, and in future ExtraTrees work, and document min_samples_leaf as weight-sum under sample_weight. |
Contributor
|
@switch527 this ended up a bit more complicated than expected and I am still thinking about the best way forward, so we can wait a bit. |
Contributor
Author
|
@RAMitchell, no worries. Let me know when you decide and how you want to proceed. |
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.
Towards #8093. Adds sample_weight to
RandomForestRegressor.fit, building on the classifier work in #8132. Single-GPU only; distributed (Dask) raisesNotImplementedError(tracking #8186).The regressor follows the companion-buffer approach the classifier already uses: a per-bin weighted-count buffer feeds the weighted gain and leaf mean, while the unweighted sample count still drives
min_samples_leaf. Weighted MSE and Poisson match sklearn; Gamma and InverseGaussian apply the same substitution to cuml's existing formulas. Leaf vectors NaN-guard the all-zero-weight case.Adds per-objective ground-truth gtests plus Python tests mirrored across RFC and RFR. The accel proxy now forwards sample_weight to the GPU path. xfail entries for sklearn's
check_sample_weight_equivalencechecks stay because cuml's quantile-binned splits don't satisfy the row-duplication assumption.On the regressor bench, double-precision MSE runs ~11% slower than the pre-PR baseline (cost of the per-sample weight accumulator); float is flat and classifier is within noise.
libcuml.sogrows by 36 KB. Adjacent fix: re-enabledcpp/bench/sg/rf_regressor.cu(was disabled with a stale FIXME); fixed a couple of compile errors and scaled the workload down so it finishes in minutes.