Add C++ RF sample weight training support - #8258
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:
📝 WalkthroughWalkthroughThis PR adds optional per-sample weighting to RandomForest and DecisionTree training. Sample weights are passed through public ChangesPer-sample weighted training
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/randomforest/randomforest.cu (1)
423-437:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWeighted
fit_treelitestill returns count-weighted feature importances.These paths now allow weighted training, but
compute_feature_importances()still scales each split gain bynode.InstanceCount(). In this PR the node counts remain raw sampled occurrences whilesample_weightonly affects impurity/objective math, so weighted models will report feature importances biased by row count instead of total weight reaching the node. That makes the exported importances wrong whenever weights are non-uniform.Also applies to: 667-680
🤖 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.cu` around lines 423 - 437, compute_feature_importances currently multiplies split gains by node.InstanceCount(), which yields count-weighted importances even when training used sample_weight; change compute_feature_importances (and the same logic used in fit_treelite paths) to weight split gains by the total sample weight that reached the node instead of InstanceCount(): aggregate per-node weight using the provided sample_weight array (and respect bootstrap_masks when sampling) and use that sum (e.g., node total weight or SumWeight()) wherever node.InstanceCount() is used to scale gains so feature importances reflect weighted training.
🤖 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/src/decisiontree/batched-levelalgo/dataset.h`:
- Around line 17-18: Update the SPDX/copyright header at the top of dataset.h so
the year range includes the current year (e.g., extend "2022" to "2022-2026" or
similar) to satisfy the repo's header checker; locate the header comment in
cpp/src/decisiontree/batched-levelalgo/dataset.h (above the declarations
including DataT and the sample_weight member) and replace the old year with the
updated range or current year.
In `@cpp/src/randomforest/randomforest.cuh`:
- Around line 127-128: The current assert only checks device residency; before
dispatching the weighted-training path (where sample_weight values are consumed
as histogram mass), add a validation step that scans the sample_weight buffer
on-device and rejects any NaN or negative values: ensure every sample_weight[i]
is finite and >= 0, and return/log an error if any invalid entries are found.
Keep the existing ASSERT(sample_weight == nullptr ||
DT::is_dev_ptr(sample_weight)) but follow it with a GPU-safe check (e.g., a
short kernel or thrust::any_of) that inspects the device buffer and fails early
if non-finite or negative weights are present so downstream uses in
histogram/weighted objective code are protected.
---
Outside diff comments:
In `@cpp/src/randomforest/randomforest.cu`:
- Around line 423-437: compute_feature_importances currently multiplies split
gains by node.InstanceCount(), which yields count-weighted importances even when
training used sample_weight; change compute_feature_importances (and the same
logic used in fit_treelite paths) to weight split gains by the total sample
weight that reached the node instead of InstanceCount(): aggregate per-node
weight using the provided sample_weight array (and respect bootstrap_masks when
sampling) and use that sum (e.g., node total weight or SumWeight()) wherever
node.InstanceCount() is used to scale gains so feature importances reflect
weighted training.
🪄 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: f0d57bf2-f2ca-4796-9c7c-ef5746f6ae37
📒 Files selected for processing (16)
cpp/CMakeLists.txtcpp/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/kernels/weighted-classification-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted-classification-float.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted-regression-double.cucpp/src/decisiontree/batched-levelalgo/kernels/weighted-regression-float.cucpp/src/decisiontree/batched-levelalgo/objectives.cuhcpp/src/decisiontree/decisiontree.cuhcpp/src/randomforest/randomforest.cucpp/src/randomforest/randomforest.cuhcpp/tests/sg/rf_test.cu
chyunsu3
left a comment
There was a problem hiding this comment.
The C++ changes look good overall.
Question. Can you point me which part of this PR address Rejects zero-weight split children through objective math.? I'm not seeing which lines of code checks for zero-weight splits.
|
/ok to test 03bb470 |
03bb470 to
ab3c6b7
Compare
|
/merge |
Summary
Refs #8093.
This PR threads optional device
sample_weightthrough the C++ Random Forest training path and into the batched-levelalgo histogram builders.It adds weighted RF objective/kernel instantiations selected when
sample_weight != nullptr. Histogram bins keepcountas the minimum-samples metadata, while weighted bins additionally accumulate sample weight for impurity/objective math and leaf values.Details
sample_weightto the C++ RF fit path and decision tree dataset plumbing.count += 1for every sampled occurrence, including bootstrap duplicates.weight += sample_weight[row]for weighted bins.min_samples_leafand related split constraints count-based.Tests
min_samples_leafcount semantics, and bootstrap duplicate contribution.Validation run:
conda run -n cuml_dev cmake --build /home/rorym/cuml-builds/codex-enh-rf-sample-weight/cpp-release --target SG_RF_TEST -j16 conda run -n cuml_dev bash -lc 'LD_PRELOAD=/home/rorym/cuml-builds/codex-enh-rf-sample-weight/cpp-release/libcuml.so /home/rorym/cuml-builds/codex-enh-rf-sample-weight/cpp-release/tests/SG_RF_TEST'SG_RF_TEST: 282/282 passed.