Center RF split thresholds across empty quantile bins - #8283
Conversation
RAMitchell
added
improvement
e5f0f5d to
386b963
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves cuML’s batched decision-tree split selection for RandomForest by carrying the winning histogram bin through reduction and re-centering inference thresholds when the best split falls within a contiguous “empty quantile bin” plateau, aiming to better match scikit-learn’s threshold placement behavior.
Changes:
- Extend
DT::Splitto retain the winning histogrambinidand refine the final threshold across empty-bin plateaus. - Update objective functions and split-reduction plumbing to propagate
binidend-to-end. - Add C++ tests covering the plateau-centering behavior for both classification and regression.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
cpp/tests/sg/rf_test.cu |
Adds GPU-side unit tests validating plateau-centered split selection for classification/regression. |
cpp/src/decisiontree/batched-levelalgo/split.cuh |
Adds binid to Split and introduces plateau-centering refinement logic. |
cpp/src/decisiontree/batched-levelalgo/objectives.cuh |
Propagates binid from per-bin evaluation into Split updates; reuses shared CountLeft. |
cpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh |
Updates evalBestSplit invocation to pass objective/hist/quantiles for refinement. |
cpp/src/decisiontree/batched-levelalgo/builder.cuh |
Updates host-side split staging/copying to include the new binid field. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This reverts commit 092e118.
2bb7b5f to
b3e69d0
Compare
|
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 split-range tracking to split selection, threads the new range fields through objective scoring, kernel execution, and host compaction, and adds CUDA tests for classification and regression cases. ChangesEquivalent split-range tracking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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: 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/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuh`:
- Around line 221-224: `computeSplitKernel` now uses static `__shared__`
`split_scratch_storage`, but `Builder::computeSplitSmemSize()` still only
accounts for the dynamic shared-memory region. Update the launch sizing/budget
check to include the `Split<DataT, IdxT>` scratch bytes explicitly, or move that
storage back under `extern __shared__` so the total per-block shared-memory
usage stays within device limits. Use the existing `computeSplitKernel`,
`split_scratch_storage`, and `computeSplitSmemSize()` symbols to keep the kernel
launch accounting consistent.
🪄 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: 6f407564-786a-4c8d-890e-7b201d5bce09
📒 Files selected for processing (5)
cpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhcpp/src/decisiontree/batched-levelalgo/objectives.cuhcpp/src/decisiontree/batched-levelalgo/split.cuhcpp/tests/sg/rf_test.cu
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/src/decisiontree/batched-levelalgo/builder.cuh`:
- Around line 511-525: Use checked arithmetic for the shared-memory launch
sizing in the builder logic: the dynamic shared-memory computation in the sizing
code around the max_n_bins/dataset.num_outputs/bin size math should be rewritten
to use ML::checked_mul and ML::checked_add for every host-side
multiplication/addition, including the later alignment and CUB/scratch size
accumulation, so overflow cannot under-budget shared memory or corrupt the
returned dynamic smem size.
🪄 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: 8467b29b-42d2-41d8-ae4a-f3592bd70c67
📒 Files selected for processing (4)
cpp/src/decisiontree/batched-levelalgo/builder.cuhcpp/src/decisiontree/batched-levelalgo/kernels/builder_kernels_impl.cuhcpp/src/decisiontree/batched-levelalgo/split.cuhcpp/tests/sg/rf_test.cu
|
@hcho3 Can you give this a first review pass, please? |
dantegd
left a comment
dantegd
left a comment
There was a problem hiding this comment.
Change looks good to me, just left a non blocking testing question
| split.evalBestSplit(split_scratch, out, mutex, quantiles, n_bins); | ||
| } | ||
|
|
||
| TEST(RFEquivalentSplitRangeTest, ClassificationChoosesUpperMiddleBin) |
There was a problem hiding this comment.
These tests cover the objective/reduction helper path, but not the PR’s end-to-end RF builder behavior. What do you think about adding a small fit/predict or tree-threshold test with quantiles that include an empty plateau, so we also exercise computeSplitKernel, the host split copy-back, node partitioning, and the threshold persisted into the tree?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/tests/sg/rf_test.cu (1)
1016-1028: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
HostSplitmirror is validated only by total size, not field layout.
static_assert(sizeof(HostSplit) == sizeof(DT::Split<...>))followed by a rawcudaMemcpyAsyncintoHostSplitrelies on the field offsets ofDT::SplitmatchingHostSplitexactly. A future field reorder/repad inSplitthat preserves size would silently make these tests read wrong members instead of failing to compile. This same block is duplicated in the regression test (Lines 1072-1083). Consider copying directly into aDT::Split<DataT, IdxT>(it is POD-compatible) or addingoffsetofassertions per field.🤖 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 1016 - 1028, The HostSplit mirror check only compares total size and then memcpy’s the device object into a differently laid out struct, so a field reorder or padding change in DT::Split<DataT, IdxT> could break the test silently. Update the rf_test.cu validation blocks around HostSplit/DT::Split to either copy directly into DT::Split<DataT, IdxT> or add explicit offsetof/static layout assertions for each member (quesval, colid, best_metric_val, nLeft, split_start, split_end), and apply the same fix to the duplicated regression test block.
🤖 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/tests/sg/rf_test.cu`:
- Around line 1016-1028: The HostSplit mirror check only compares total size and
then memcpy’s the device object into a differently laid out struct, so a field
reorder or padding change in DT::Split<DataT, IdxT> could break the test
silently. Update the rf_test.cu validation blocks around HostSplit/DT::Split to
either copy directly into DT::Split<DataT, IdxT> or add explicit offsetof/static
layout assertions for each member (quesval, colid, best_metric_val, nLeft,
split_start, split_end), and apply the same fix to the duplicated regression
test block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b89f1f1-2351-46f7-8e71-b14c486a9369
📒 Files selected for processing (1)
cpp/tests/sg/rf_test.cu
|
/merge |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
/ok to test e367ccb |
Summary
Addresses #6416.
This updates the batched decision-tree builder so the selected split keeps track of the histogram bin that produced the best gain. When a best split lands inside a run of empty quantile bins, the training partition and gain are identical across that run, but the stored inference threshold is not. We now recenter the selected split to the middle bin of that equivalent plateau instead of leaving the threshold at an edge.
This is intended to better match sklearn's exact-tree behavior, where stored thresholds sit between observed values rather than at an observed endpoint.
Changes
binidtoSplitso the winning quantile bin survives reduction and host copies.Experiments
All accuracy intervals below are 95% CI half-widths over 10 randomized train/test splits.
Original CoverType reproduction on latest main:
95.5372% +/- 0.0318%95.3727% +/- 0.0341%+0.1645 +/- 0.0305percentage pointsIncreasing bins did not remove the gap:
n_bins=512:95.4154% +/- 0.0426%+0.1219 +/- 0.0350percentage pointsQuantizing the input to 256 per-feature values also did not remove it:
95.5128% +/- 0.0293%n_bins=512:95.4097% +/- 0.0535%+0.1031 +/- 0.0289percentage pointsA post-hoc node-local midpoint diagnostic almost eliminated the CoverType gap:
95.3511% +/- 0.0479%95.5349% +/- 0.0525%+0.0023 +/- 0.0379percentage pointsWith this empty-bin plateau-centering strategy on CoverType:
95.5359% +/- 0.0456%95.5709% +/- 0.0344%-0.0349 +/- 0.0465percentage pointsn_bins=512:95.7071% +/- 0.0349%n_bins=512gap:-0.1712 +/- 0.0365percentage pointsA broader 5-dataset prototype run with 10 randomized splits did not show a remaining consistent cuML deficit across CoverType, phoneme, spambase, satimage, and a synthetic hard classification dataset.
Validation
pre-commitpassed on the committed files.SG_RF_TEST; the edited CUDA objects compiled, but final link hit the existing local mixed-toolchain/nvforest link failure.