Implement weighted bootstrap sampling for RF - #8296
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
RAMitchell
added
improvement
There was a problem hiding this comment.
Pull request overview
This PR implements sklearn-style weighted bootstrap sampling for the C++ RandomForest training path by sampling row IDs according to sample_weight when bootstrap=true, while standardizing RF sample weights to double across float/double forests and removing the bootstrap_masks plumbing.
Changes:
- Introduces an internal
detail::RowSamplerthat validates weights, builds a weighted CDF, and generates per-tree sampled row IDs (weighted or unweighted). - Changes RF/DT APIs to take
const double* sample_weightand removes thebootstrap_masksoutput parameter. - Updates C++ SG RF tests to use
doubleweights and validate weighted-bootstrap behavior via trained trees rather than bootstrap masks.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cpp/tests/sg/rf_test.cu |
Updates tests to use double sample weights and adds coverage for weighted bootstrap sampling semantics. |
cpp/src/randomforest/randomforest.cuh |
Adds RowSampler and rewires RF training to use sampled row IDs and conditional weight propagation. |
cpp/src/randomforest/randomforest.cu |
Updates exported fit / fit_treelite entry points to the new double* sample weight signature and removes bootstrap_masks. |
cpp/src/decisiontree/decisiontree.cuh |
Updates DT fit signature to accept const double* sample_weight. |
cpp/src/decisiontree/batched-levelalgo/dataset.h |
Changes dataset sample weight storage to const double*. |
cpp/src/decisiontree/batched-levelalgo/builder.cuh |
Threads const double* sample_weight through the batched builder constructor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRandomForest and DecisionTree training interfaces now take ChangesRandomForest sample-weight plumbing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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)
145-177: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftHIGH: Add migration coverage for this public fit signature break.
Removing
bootstrap_masksand changing float-forestsample_weighttoconst double*breaks existing callers of thesecpp/include/cumldeclarations. Please add a migration/docs note or compatibility/deprecation path for the old signature.As per path instructions, “Breaking changes require deprecation warnings and migration guide updates.”
Also applies to: 227-256
🤖 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 - 177, The public fit API in randomforest.hpp has a breaking signature change, so update the migration/docs path and provide compatibility or deprecation coverage for existing callers of fit and fit_treelite. Add deprecation warnings or an overload/adaptor for the old bootstrap_masks-based signature and the previous float-forest sample_weight form, and document the migration in the relevant public API notes so users can transition safely.Sources: Coding guidelines, Path instructions
🤖 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/randomforest/randomforest.cuh`:
- Around line 64-73: The weighted bootstrap path in randomforest.cuh can produce
an out-of-range row index because sample_weight_sum_ is derived separately from
sample_weight_cdf_ and upper_bound may return n_rows_ if the sum exceeds the
scan tail due to roundoff. Update the weighted draw logic in the constructor and
the selection code paths (the bootstrap sampling and downstream uses of
selected_rows) to clamp or derive the search bound from
sample_weight_cdf_.back() instead of the independent sum, so generated indices
always stay within [0, n_rows_ - 1].
---
Outside diff comments:
In `@cpp/include/cuml/ensemble/randomforest.hpp`:
- Around line 145-177: The public fit API in randomforest.hpp has a breaking
signature change, so update the migration/docs path and provide compatibility or
deprecation coverage for existing callers of fit and fit_treelite. Add
deprecation warnings or an overload/adaptor for the old bootstrap_masks-based
signature and the previous float-forest sample_weight form, and document the
migration in the relevant public API notes so users can transition safely.
🪄 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: d8ae3902-55c0-46be-8b68-c25c001f405d
📒 Files selected for processing (7)
cpp/include/cuml/ensemble/randomforest.hppcpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/dataset.hcpp/src/decisiontree/decisiontree.cuhcpp/src/randomforest/randomforest.cucpp/src/randomforest/randomforest.cuhcpp/tests/sg/rf_test.cu
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/include/cuml/ensemble/randomforest.hpp (1)
145-180: 🗄️ Data Integrity & Integration | 🟠 MajorPreserve the existing
sample_weightsignature
Changing the public RFfit/fit_treeliteoverloads toconst double* sample_weightbreaks callers that passfloat*. Add a deprecated compatibility overload or migration note for the new type.🤖 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 - 180, The public random forest fit overloads and fit_treelite now take sample_weight as const double*, which breaks existing callers that pass float*; update the API in randomforest.hpp to preserve backward compatibility by adding a deprecated shim/overload for the old float* signature or an equivalent compatibility path. Keep the new const double* form in fit and fit_treelite, and make sure the compatibility overload forwards to the new implementation so existing call sites continue to compile.Source: Path instructions
🤖 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/randomforest/randomforest.cuh`:
- Around line 134-147: `store_bootstrap_mask()` in `randomforest.cuh` writes to
`bootstrap_masks_` without validating that the device buffer is usable, so add a
guard before the Thrust fill/scatter path to ensure `bootstrap_masks_` is
non-null and points to valid device memory for the current tree; if the pointer
is invalid, fail fast or skip the write. Keep the check close to the
`store_bootstrap_mask`/`bootstrap_masks_` logic so the device writes are only
launched when `RowSampler` has a safe mask buffer.
- Line 140: The tree mask pointer offset in the bootstrap mask handling is
computed with unchecked host-side multiplication, which can wrap and corrupt the
destination mask. Update the offset calculation in the random forest mask logic
that uses bootstrap_masks_ and tree_id to use the checked arithmetic helpers for
host-side count arithmetic before forming the tree_mask pointer. Keep the fix
localized to the tree mask row-offset computation so overflow is detected
instead of silently wrapping.
---
Outside diff comments:
In `@cpp/include/cuml/ensemble/randomforest.hpp`:
- Around line 145-180: The public random forest fit overloads and fit_treelite
now take sample_weight as const double*, which breaks existing callers that pass
float*; update the API in randomforest.hpp to preserve backward compatibility by
adding a deprecated shim/overload for the old float* signature or an equivalent
compatibility path. Keep the new const double* form in fit and fit_treelite, and
make sure the compatibility overload forwards to the new implementation so
existing call sites continue to compile.
🪄 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: 1aa170ce-5bde-44ba-86f0-67eeef9cdccd
📒 Files selected for processing (4)
cpp/include/cuml/ensemble/randomforest.hppcpp/src/randomforest/randomforest.cucpp/src/randomforest/randomforest.cuhcpp/tests/sg/rf_test.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/tests/sg/rf_test.cu
|
/merge |
Summary
doubleinternally, including float forests.RowSamplerthat owns validation, CDF construction, and per-stream sampling buffers.bootstrap_masksRF API/plumbing and update tests to validate behavior through trained trees instead.Details
Weighted bootstrap now uses
sample_weightas draw probabilities. When bootstrap is enabled, trees train from the sampled row IDs/counts rather than receiving the original sample weights, matching sklearn behavior.RowSamplerhandles sample-weight validation and sum computation, weighted CDF construction, per-stream selected row buffers, weighted draw scratch storage, and bootstrap/non-bootstrap row-id generation.Tests
cmake --build /home/rorym/cuml-builds/codex-enh-rf-weighted-bootstrap/cpp-release --target SG_RF_TEST -j 8/home/rorym/cuml-builds/codex-enh-rf-weighted-bootstrap/cpp-release/tests/SG_RF_TEST --gtest_filter=RfWeightedTest.*:WeightedObjectiveEdgeCases.*:RfTests.InvalidSampleWeightThrows:RfTests.WeightedBootstrapSamplesOnlyPositiveWeightRows/home/rorym/cuml-builds/codex-enh-rf-weighted-bootstrap/cpp-release/tests/SG_RF_TEST --gtest_filter=RfTests/RfTest.PropertyBasedTest/*No Python changes.